Skip to content

Commit

Permalink
address CR comments
Browse files Browse the repository at this point in the history
  • Loading branch information
MikeHolman committed Sep 16, 2016
1 parent 32f729b commit 351fc5c
Show file tree
Hide file tree
Showing 39 changed files with 310 additions and 240 deletions.
7 changes: 6 additions & 1 deletion bin/ChakraCore/ChakraCoreDllFunc.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -213,4 +213,9 @@ HRESULT JsInitializeJITServer(
{
return E_NOTIMPL;
}
#endif
EXPORT_FUNC
HRESULT JsShutdownJITServer()
{
return E_NOTIMPL;
}
#endif
10 changes: 1 addition & 9 deletions bin/ch/JITProcessManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ HRESULT JITProcessManager::CreateServerProcess(int argc, __in_ecount(argc) LPWST
NULL,
NULL,
FALSE,
CREATE_SUSPENDED,
NULL,
NULL,
NULL,
&si,
Expand All @@ -116,14 +116,6 @@ HRESULT JITProcessManager::CreateServerProcess(int argc, __in_ecount(argc) LPWST

free(cmdLine);

if (ResumeThread(processInfo.hThread) == (DWORD)-1)
{
TerminateProcess(processInfo.hProcess, GetLastError());
CloseHandle(processInfo.hProcess);
CloseHandle(processInfo.hThread);
return HRESULT_FROM_WIN32(GetLastError());
}

CloseHandle(processInfo.hThread);
s_rpcServerProcessHandle = processInfo.hProcess;

Expand Down
2 changes: 2 additions & 0 deletions bin/ch/ch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,9 @@ bool HandleJITServerFlag(int& argc, _Inout_updates_to_(argc, argc) LPWSTR argv[]
}

if (i == argc)
{
return false;
}

// remove this flag now
HostConfigFlags::RemoveArg(argc, argv, i);
Expand Down
2 changes: 0 additions & 2 deletions bin/ch/ch.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@
kernel32.lib;
Rpcrt4.lib;
</AdditionalDependencies>
<IgnoreAllDefaultLibraries Condition="'$(Configuration)|$(Platform)'=='Debug|x64'">
</IgnoreAllDefaultLibraries>
</Link>
</ItemDefinitionGroup>
<ItemGroup>
Expand Down
63 changes: 33 additions & 30 deletions lib/Backend/BackwardPass.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -710,7 +710,7 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
auto fixupFrom = [block, blockSucc, this](Bucket<AddPropertyCacheBucket> &bucket)
{
AddPropertyCacheBucket *fromData = &bucket.element;
if (fromData->GetInitialType().t == nullptr ||
if (fromData->GetInitialType() == nullptr ||
fromData->GetFinalType() == fromData->GetInitialType())
{
return;
Expand All @@ -722,7 +722,7 @@ BackwardPass::MergeSuccBlocksInfo(BasicBlock * block)
auto fixupTo = [blockSucc, this](Bucket<AddPropertyCacheBucket> &bucket)
{
AddPropertyCacheBucket *toData = &bucket.element;
if (toData->GetInitialType().t == nullptr ||
if (toData->GetInitialType() == nullptr ||
toData->GetFinalType() == toData->GetInitialType())
{
return;
Expand Down Expand Up @@ -1184,7 +1184,7 @@ BackwardPass::MergeGuardedProperties(ObjTypeGuardBucket bucket1, ObjTypeGuardBuc
ObjTypeGuardBucket bucket;
bucket.SetGuardedPropertyOps(mergedPropertyOps);
JITTypeHolder monoGuardType = bucket1.GetMonoGuardType();
if (monoGuardType.t != nullptr)
if (monoGuardType != nullptr)
{
Assert(!bucket2.NeedsMonoCheck() || monoGuardType == bucket2.GetMonoGuardType());
}
Expand Down Expand Up @@ -3662,7 +3662,7 @@ BackwardPass::ProcessNewScObject(IR::Instr* instr)
// transition here.
AddPropertyCacheBucket *pBucket = block->stackSymToFinalType->Get(objSym->m_id);
if (pBucket &&
pBucket->GetInitialType().t != nullptr &&
pBucket->GetInitialType() != nullptr &&
pBucket->GetFinalType() != pBucket->GetInitialType())
{
Assert(pBucket->GetInitialType() == ctorCache->GetType());
Expand All @@ -3672,7 +3672,7 @@ BackwardPass::ProcessNewScObject(IR::Instr* instr)
}
#if DBG
pBucket->deadStoreUnavailableInitialType = pBucket->GetInitialType();
if (pBucket->deadStoreUnavailableFinalType.t == nullptr)
if (pBucket->deadStoreUnavailableFinalType == nullptr)
{
pBucket->deadStoreUnavailableFinalType = pBucket->GetFinalType();
}
Expand Down Expand Up @@ -4256,7 +4256,7 @@ BackwardPass::ProcessPropertySymOpndUse(IR::PropertySymOpnd * opnd)
StackSym *baseSym = opnd->GetObjectSym();
AddPropertyCacheBucket *pBucket = block->stackSymToFinalType->Get(baseSym->m_id);
if (pBucket &&
pBucket->GetFinalType().t != nullptr &&
pBucket->GetFinalType() != nullptr &&
pBucket->GetFinalType() != pBucket->GetInitialType())
{
this->InsertTypeTransition(this->currentInstr->m_next, baseSym, pBucket);
Expand Down Expand Up @@ -4501,7 +4501,7 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block
JITTypeHolder typeWithProperty = opnd->IsMono() ? opnd->GetType() : opnd->GetFirstEquivalentType();
JITTypeHolder typeWithoutProperty = opnd->HasInitialType() ? opnd->GetInitialType() : JITTypeHolder(nullptr);

if (typeWithoutProperty.t == nullptr ||
if (typeWithoutProperty == nullptr ||
typeWithProperty == typeWithoutProperty ||
(opnd->IsTypeChecked() && !opnd->IsInitialTypeChecked()))
{
Expand All @@ -4510,7 +4510,7 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block
PropertySym *propertySym = opnd->m_sym->AsPropertySym();
AddPropertyCacheBucket *pBucket =
block->stackSymToFinalType->Get(propertySym->m_stackSym->m_id);
if (pBucket && pBucket->GetFinalType().t && pBucket->GetInitialType() != pBucket->GetFinalType())
if (pBucket && pBucket->GetFinalType() != nullptr && pBucket->GetInitialType() != pBucket->GetFinalType())
{
opnd->SetFinalType(pBucket->GetFinalType());
}
Expand All @@ -4519,12 +4519,13 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block
return;
}

#if 0 // TODO: OOP JIT, add these assert back (ReadProcessMemory?)
#if DBG
Assert(typeWithProperty != nullptr);
Js::DynamicTypeHandler * typeWithoutPropertyTypeHandler = static_cast<Js::DynamicType *>(typeWithoutProperty)->GetTypeHandler();
Js::DynamicTypeHandler * typeWithPropertyTypeHandler = static_cast<Js::DynamicType *>(typeWithProperty)->GetTypeHandler();
Assert(typeWithoutPropertyTypeHandler->GetPropertyCount() + 1 == typeWithPropertyTypeHandler->GetPropertyCount());
AssertMsg(Js::DynamicObject::IsTypeHandlerCompatibleForObjectHeaderInlining(typeWithoutPropertyTypeHandler, typeWithPropertyTypeHandler),
const JITTypeHandler * typeWithoutPropertyTypeHandler = typeWithoutProperty->GetTypeHandler();
const JITTypeHandler * typeWithPropertyTypeHandler = typeWithProperty->GetTypeHandler();
// TODO: OOP JIT, reenable assert
//Assert(typeWithoutPropertyTypeHandler->GetPropertyCount() + 1 == typeWithPropertyTypeHandler->GetPropertyCount());
AssertMsg(JITTypeHandler::IsTypeHandlerCompatibleForObjectHeaderInlining(typeWithoutPropertyTypeHandler, typeWithPropertyTypeHandler),
"TypeHandlers are not compatible for transition?");
Assert(typeWithoutPropertyTypeHandler->GetSlotCapacity() <= typeWithPropertyTypeHandler->GetSlotCapacity());
#endif
Expand All @@ -4545,7 +4546,7 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block
#if DBG
JITTypeHolder deadStoreUnavailableFinalType(nullptr);
#endif
if (pBucket->GetInitialType().t == nullptr || opnd->GetType() != pBucket->GetInitialType())
if (pBucket->GetInitialType() == nullptr || opnd->GetType() != pBucket->GetInitialType())
{
#if DBG
if (opnd->GetType() == pBucket->deadStoreUnavailableInitialType)
Expand All @@ -4571,7 +4572,7 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block

if (!PHASE_OFF(Js::ObjTypeSpecStorePhase, this->func))
{
#if 0 //TODO: OOP JIT, reenable assert
#if DBG

// We may regress in this case:
// if (b)
Expand All @@ -4592,15 +4593,17 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block
if (!opnd->IsTypeDead())
{
// This is the type that would have been propagated if we didn't kill it because the type isn't available
JITTypeHolder checkFinalType = deadStoreUnavailableFinalType ? deadStoreUnavailableFinalType : finalType;
JITTypeHolder checkFinalType = deadStoreUnavailableFinalType != nullptr ? deadStoreUnavailableFinalType : finalType;
if (opnd->HasFinalType() && opnd->GetFinalType() != checkFinalType)
{
// Final type discovery must be progressively better (unless we kill it in the deadstore pass
// when the type is not available during the forward pass)
Js::DynamicTypeHandler * oldFinalTypeHandler = static_cast<Js::DynamicType *>(opnd->GetFinalType())->GetTypeHandler();
Js::DynamicTypeHandler * checkFinalTypeHandler = static_cast<Js::DynamicType *>(checkFinalType)->GetTypeHandler();
Assert(oldFinalTypeHandler->GetPropertyCount() < checkFinalTypeHandler->GetPropertyCount());
AssertMsg(Js::DynamicObject::IsTypeHandlerCompatibleForObjectHeaderInlining(oldFinalTypeHandler, checkFinalTypeHandler),
const JITTypeHandler * oldFinalTypeHandler = opnd->GetFinalType()->GetTypeHandler();
const JITTypeHandler * checkFinalTypeHandler = checkFinalType->GetTypeHandler();

// TODO: OOP JIT, enable assert
//Assert(oldFinalTypeHandler->GetPropertyCount() < checkFinalTypeHandler->GetPropertyCount());
AssertMsg(JITTypeHandler::IsTypeHandlerCompatibleForObjectHeaderInlining(oldFinalTypeHandler, checkFinalTypeHandler),
"TypeHandlers should be compatible for transition.");
Assert(oldFinalTypeHandler->GetSlotCapacity() <= checkFinalTypeHandler->GetSlotCapacity());
}
Expand Down Expand Up @@ -4644,7 +4647,7 @@ BackwardPass::TrackAddPropertyTypes(IR::PropertySymOpnd *opnd, BasicBlock *block
{
#if DBG
pBucket->deadStoreUnavailableInitialType = pBucket->GetInitialType();
if (pBucket->deadStoreUnavailableFinalType.t == nullptr)
if (pBucket->deadStoreUnavailableFinalType == nullptr)
{
pBucket->deadStoreUnavailableFinalType = pBucket->GetFinalType();
}
Expand All @@ -4671,11 +4674,11 @@ BackwardPass::InsertTypeTransition(IR::Instr *instrInsertBefore, StackSym *objSy
baseOpnd->SetIsJITOptimizedReg(true);

IR::AddrOpnd *initialTypeOpnd =
IR::AddrOpnd::New(data->GetInitialType().t->GetAddr(), IR::AddrOpndKindDynamicType, this->func);
IR::AddrOpnd::New(data->GetInitialType()->GetAddr(), IR::AddrOpndKindDynamicType, this->func);
initialTypeOpnd->m_metadata = data->GetInitialType().t;

IR::AddrOpnd *finalTypeOpnd =
IR::AddrOpnd::New(data->GetFinalType().t->GetAddr(), IR::AddrOpndKindDynamicType, this->func);
IR::AddrOpnd::New(data->GetFinalType()->GetAddr(), IR::AddrOpndKindDynamicType, this->func);
finalTypeOpnd->m_metadata = data->GetFinalType().t;

IR::Instr *adjustTypeInstr =
Expand Down Expand Up @@ -4722,8 +4725,8 @@ BackwardPass::InsertTypeTransitionAtBlock(BasicBlock *block, int symId, AddPrope
{
// This symbol already has a type transition at this point.
// It *must* be doing the same transition we're already trying to do.
Assert((intptr_t)instr->GetDst()->AsAddrOpnd()->m_address == data->GetFinalType().t->GetAddr() &&
(intptr_t)instr->GetSrc2()->AsAddrOpnd()->m_address == data->GetInitialType().t->GetAddr());
Assert((intptr_t)instr->GetDst()->AsAddrOpnd()->m_address == data->GetFinalType()->GetAddr() &&
(intptr_t)instr->GetSrc2()->AsAddrOpnd()->m_address == data->GetInitialType()->GetAddr());
// Nothing to do.
return;
}
Expand Down Expand Up @@ -4824,7 +4827,7 @@ BackwardPass::ForEachAddPropertyCacheBucket(Fn fn)
FOREACH_HASHTABLE_ENTRY(AddPropertyCacheBucket, bucket, block->stackSymToFinalType)
{
AddPropertyCacheBucket *data = &bucket.element;
if (data->GetInitialType().t != nullptr &&
if (data->GetInitialType() != nullptr &&
data->GetInitialType() != data->GetFinalType())
{
bool done = fn(bucket.value, data);
Expand All @@ -4841,22 +4844,22 @@ bool
BackwardPass::TransitionUndoesObjectHeaderInlining(AddPropertyCacheBucket *data) const
{
JITTypeHolder type = data->GetInitialType();
if (type.t == nullptr || !Js::DynamicType::Is(type.t->GetTypeId()))
if (type == nullptr || !Js::DynamicType::Is(type->GetTypeId()))
{
return false;
}

if (!type.t->GetTypeHandler()->IsObjectHeaderInlinedTypeHandler())
if (!type->GetTypeHandler()->IsObjectHeaderInlinedTypeHandler())
{
return false;
}

type = data->GetFinalType();
if (type.t == nullptr || !Js::DynamicType::Is(type.t->GetTypeId()))
if (type == nullptr || !Js::DynamicType::Is(type->GetTypeId()))
{
return false;
}
return !type.t->GetTypeHandler()->IsObjectHeaderInlinedTypeHandler();
return !type->GetTypeHandler()->IsObjectHeaderInlinedTypeHandler();
}

void
Expand Down
12 changes: 4 additions & 8 deletions lib/Backend/Encoder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -271,8 +271,6 @@ Encoder::Encode()
}
}

JITTimeWorkItem * workItem = m_func->GetWorkItem();

BEGIN_CODEGEN_PHASE(m_func, Js::EmitterPhase);

// Copy to permanent buffer.
Expand Down Expand Up @@ -440,7 +438,7 @@ Encoder::Encode()
equivalentTypeGuardOffsets->guards[i].cache.nextEvictionVictim = cache->nextEvictionVictim;
equivalentTypeGuardOffsets->guards[i].cache.record.propertyCount = cache->record.propertyCount;
equivalentTypeGuardOffsets->guards[i].cache.record.propertyOffset = NativeCodeData::GetDataTotalOffset(cache->record.properties);
for (int j = 0; j < EQUIVALENT_TYPE_CACHE_SIZE_IDL; j++)
for (int j = 0; j < EQUIVALENT_TYPE_CACHE_SIZE; j++)
{
equivalentTypeGuardOffsets->guards[i].cache.types[j] = (intptr_t)cache->types[j];
}
Expand Down Expand Up @@ -477,10 +475,8 @@ Encoder::Encode()
if (this->m_func->propertyGuardsByPropertyId != nullptr)
{
Assert(!isSimpleJit);
# if 0 // TODO: OOP JIT, is this assert valid?
AssertMsg(!(PHASE_OFF(Js::ObjTypeSpecPhase, this->m_func) && PHASE_OFF(Js::FixedMethodsPhase, this->m_func)),
"Why do we have type guards if we don't do object type spec or fixed methods?");
#endif

#if DBG
int totalGuardCount = (this->m_func->singleTypeGuards != nullptr ? this->m_func->singleTypeGuards->Count() : 0)
Expand Down Expand Up @@ -687,10 +683,10 @@ Encoder::Encode()
Js::Configuration::Global.flags.DumpIRAddresses = dumpIRAddressesValue;
}

if (PHASE_DUMP(Js::EncoderPhase, m_func) && Js::Configuration::Global.flags.Verbose)
if (PHASE_DUMP(Js::EncoderPhase, m_func) && Js::Configuration::Global.flags.Verbose && !m_func->IsOOPJIT())
{
workItem->DumpNativeOffsetMaps();
workItem->DumpNativeThrowSpanSequence();
m_func->GetInProcJITEntryPointInfo()->DumpNativeOffsetMaps();
m_func->GetInProcJITEntryPointInfo()->DumpNativeThrowSpanSequence();
this->DumpInlineeFrameMap(m_func->GetJITOutput()->GetCodeAddress());
Output::Flush();
}
Expand Down
6 changes: 3 additions & 3 deletions lib/Backend/FlowGraph.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3354,9 +3354,9 @@ BasicBlock::Dump()
void
AddPropertyCacheBucket::Dump() const
{
Assert(this->initialType.t != nullptr);
Assert(this->finalType.t != nullptr);
Output::Print(_u(" initial type: 0x%x, final type: 0x%x "), this->initialType.t->GetAddr(), this->finalType.t->GetAddr());
Assert(this->initialType != nullptr);
Assert(this->finalType != nullptr);
Output::Print(_u(" initial type: 0x%x, final type: 0x%x "), this->initialType->GetAddr(), this->finalType->GetAddr());
}

void
Expand Down
2 changes: 1 addition & 1 deletion lib/Backend/FlowGraph.h
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ class ObjTypeGuardBucket
void SetGuardedPropertyOps(BVSparse<JitArenaAllocator> *guardedPropertyOps) { this->guardedPropertyOps = guardedPropertyOps; }
void AddToGuardedPropertyOps(uint propertyOpId) { Assert(this->guardedPropertyOps != nullptr); this->guardedPropertyOps->Set(propertyOpId); }

bool NeedsMonoCheck() const { return this->monoGuardType.t != nullptr; }
bool NeedsMonoCheck() const { return this->monoGuardType != nullptr; }
void SetMonoGuardType(JITTypeHolder type) { this->monoGuardType = type; }
JITTypeHolder GetMonoGuardType() const { return this->monoGuardType; }

Expand Down
11 changes: 4 additions & 7 deletions lib/Backend/Func.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -804,11 +804,8 @@ Func::AjustLocalVarSlotOffset()
int localsOffset = m_localVarSlotsOffset - (m_localStackHeight + m_ArgumentsOffset);
int valueChangeOffset = m_hasLocalVarChangedOffset - (m_localStackHeight + m_ArgumentsOffset);

Js::FunctionEntryPointInfo * entryPointInfo = static_cast<Js::FunctionEntryPointInfo*>(this->m_workItem->GetEntryPoint());
Assert(entryPointInfo != nullptr);

entryPointInfo->localVarSlotsOffset = localsOffset;
entryPointInfo->localVarChangedOffset = valueChangeOffset;
m_output.SetVarSlotsOffset(localsOffset);
m_output.SetVarChangedOffset(valueChangeOffset);
}
}
#endif
Expand Down Expand Up @@ -1389,7 +1386,7 @@ Func::CreateEquivalentTypeGuard(JITTypeHolder type, uint32 objTypeSpecFldId)
{
EnsureEquivalentTypeGuards();

Js::JitEquivalentTypeGuard* guard = NativeCodeDataNew(GetNativeCodeDataAllocator(), Js::JitEquivalentTypeGuard, type.t->GetAddr(), this->indexedPropertyGuardCount++, objTypeSpecFldId);
Js::JitEquivalentTypeGuard* guard = NativeCodeDataNewNoFixup(GetNativeCodeDataAllocator(), Js::JitEquivalentTypeGuard, type->GetAddr(), this->indexedPropertyGuardCount++, objTypeSpecFldId);

// If we want to hard code the address of the cache, we will need to go back to allocating it from the native code data allocator.
// We would then need to maintain consistency (double write) to both the recycler allocated cache and the one on the heap.
Expand All @@ -1400,7 +1397,7 @@ Func::CreateEquivalentTypeGuard(JITTypeHolder type, uint32 objTypeSpecFldId)
}
else
{
cache = NativeCodeDataNewZ(GetTransferDataAllocator(), Js::EquivalentTypeCache);
cache = NativeCodeDataNewZNoFixup(GetTransferDataAllocator(), Js::EquivalentTypeCache);
}
guard->SetCache(cache);

Expand Down
4 changes: 2 additions & 2 deletions lib/Backend/GlobOpt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3506,7 +3506,7 @@ JsTypeValueInfo* GlobOpt::MergeJsTypeValueInfo(JsTypeValueInfo * toValueInfo, Js
return toValueInfo;
}

if (mergedType.t == nullptr && mergedTypeSet == nullptr)
if (mergedType == nullptr && mergedTypeSet == nullptr)
{
// No info, so don't bother making a value.
return nullptr;
Expand Down Expand Up @@ -20534,7 +20534,7 @@ void ValueInfo::Dump()
else if(IsJsType())
{
const JITTypeHolder type(AsJsType()->GetJsType());
type.t != nullptr ? Output::Print(_u("type: 0x%p, "), type.t->GetAddr()) : Output::Print(_u("type: null, "));
type != nullptr ? Output::Print(_u("type: 0x%p, "), type->GetAddr()) : Output::Print(_u("type: null, "));
Output::Print(_u("type Set: "));
Js::EquivalentTypeSet* typeSet = AsJsType()->GetJsTypeSet();
if (typeSet != nullptr)
Expand Down
Loading

0 comments on commit 351fc5c

Please sign in to comment.