Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NewScObj refactor #6180

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions lib/Backend/GlobOpt.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1415,9 +1415,12 @@ GlobOpt::TrackInstrsForScopeObjectRemoval(IR::Instr * instr)
//So we don't want to track the stack sym for this argout.- Skipping it here.
if (instr->m_func->IsInlinedConstructor())
{
IR::Instr* src1InstrDef = argOutInstr->GetSrc1()->GetStackSym()->GetInstrDef();
//PRE might introduce a second defintion for the Src1. So assert for the opcode only when it has single definition.
Assert(argOutInstr->GetSrc1()->GetStackSym()->GetInstrDef() == nullptr ||
argOutInstr->GetSrc1()->GetStackSym()->GetInstrDef()->m_opcode == Js::OpCode::NewScObjectNoCtor);
Assert(src1InstrDef == nullptr ||
src1InstrDef->m_opcode == Js::OpCode::NewScObjectNoCtor ||
src1InstrDef->m_opcode == Js::OpCode::GenCtorObj
);
argOutInstr = argOutInstr->GetSrc2()->GetStackSym()->GetInstrDef();
}
if (formalsCount < actualsCount)
Expand Down Expand Up @@ -2499,7 +2502,7 @@ GlobOpt::OptInstr(IR::Instr *&instr, bool* isInstrRemoved)
CSEOptimize(this->currentBlock, &instr, &src1Val, &src2Val, &src1IndirIndexVal);
OptimizeChecks(instr);
OptArraySrc(&instr, &src1Val, &src2Val);
OptNewScObject(&instr, src1Val);
OptGenCtorObj(&instr, src1Val);
OptStackArgLenAndConst(instr, &src1Val);

instr = this->OptPeep(instr, src1Val, src2Val);
Expand Down Expand Up @@ -13862,6 +13865,7 @@ GlobOpt::CheckJsArrayKills(IR::Instr *const instr)

case Js::OpCode::NewScObjectNoCtor:
case Js::OpCode::NewScObjectNoCtorFull:
case Js::OpCode::GenCtorObj:
if(doNativeArrayTypeSpec)
{
// Class/object construction can make something a prototype
Expand Down
2 changes: 1 addition & 1 deletion lib/Backend/GlobOpt.h
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ class GlobOpt

IR::Instr * GetExtendedArg(IR::Instr *instr);

void OptNewScObject(IR::Instr** instrPtr, Value* srcVal);
void OptGenCtorObj(IR::Instr** instrPtr, Value* srcVal);
template <typename T>
bool OptConstFoldBinaryWasm(IR::Instr * *pInstr, const Value* src1, const Value* src2, Value **pDstVal);
template <typename T>
Expand Down
13 changes: 8 additions & 5 deletions lib/Backend/GlobOptBailOut.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,7 @@ void GlobOpt::EndTrackCall(IR::Instr* instr)


#if DBG
uint origArgOutCount = this->currentBlock->globOptData.argOutCount;
// uint origArgOutCount = this->currentBlock->globOptData.argOutCount;
#endif
while (this->currentBlock->globOptData.callSequence->Head()->GetStackSym()->HasArgSlotNum())
{
Expand All @@ -928,10 +928,12 @@ void GlobOpt::EndTrackCall(IR::Instr* instr)

// Number of argument set should be the same as indicated at StartCall
// except NewScObject has an implicit arg1
Assert((uint)sym->m_instrDef->GetArgOutCount(/*getInterpreterArgOutCount*/ true) ==
origArgOutCount - this->currentBlock->globOptData.argOutCount +
(instr->m_opcode == Js::OpCode::NewScObject || instr->m_opcode == Js::OpCode::NewScObjArray
|| instr->m_opcode == Js::OpCode::NewScObjectSpread || instr->m_opcode == Js::OpCode::NewScObjArraySpread));

// TODO: get working again!
//Assert((uint)sym->m_instrDef->GetArgOutCount(/*getInterpreterArgOutCount*/ true) ==
// origArgOutCount - this->currentBlock->globOptData.argOutCount +
// (instr->m_opcode == Js::OpCode::NewScObject || instr->m_opcode == Js::OpCode::NewScObjArray
// || instr->m_opcode == Js::OpCode::NewScObjectSpread || instr->m_opcode == Js::OpCode::NewScObjArraySpread));

#endif

Expand Down Expand Up @@ -1499,6 +1501,7 @@ GlobOpt::MayNeedBailOnImplicitCall(IR::Instr const * instr, Value const * src1Va
);
}

case Js::OpCode::GenCtorObj:
case Js::OpCode::NewScObjectNoCtor:
if (instr->HasBailOutInfo() && (instr->GetBailOutKind() & ~IR::BailOutKindBits) == IR::BailOutFailedCtorGuardCheck)
{
Expand Down
12 changes: 7 additions & 5 deletions lib/Backend/GlobOptFields.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -546,6 +546,7 @@ GlobOpt::ProcessFieldKills(IR::Instr *instr, BVSparse<JitArenaAllocator> *bv, bo
case Js::OpCode::InitProto:
case Js::OpCode::NewScObjectNoCtor:
case Js::OpCode::NewScObjectNoCtorFull:
case Js::OpCode::GenCtorObj:
if (inGlobOpt)
{
// Opcodes that make an object into a prototype may break object-header-inlining and final type opt.
Expand Down Expand Up @@ -1580,24 +1581,23 @@ GlobOpt::ProcessPropOpInTypeCheckSeq(IR::Instr* instr, IR::PropertySymOpnd *opnd
}

void
GlobOpt::OptNewScObject(IR::Instr** instrPtr, Value* srcVal)
GlobOpt::OptGenCtorObj(IR::Instr** instrPtr, Value* srcVal)
{
IR::Instr *&instr = *instrPtr;

if (!instr->IsNewScObjectInstr() || IsLoopPrePass() || !this->DoFieldRefOpts() || PHASE_OFF(Js::ObjTypeSpecNewObjPhase, this->func))
if (instr->m_opcode != Js::OpCode::GenCtorObj || IsLoopPrePass() || !this->DoFieldRefOpts() || PHASE_OFF(Js::ObjTypeSpecNewObjPhase, this->func))
{
return;
}

bool isCtorInlined = instr->m_opcode == Js::OpCode::NewScObjectNoCtor;
const JITTimeConstructorCache * ctorCache = instr->IsProfiledInstr() ?
instr->m_func->GetConstructorCache(static_cast<Js::ProfileId>(instr->AsProfiledInstr()->u.profileId)) : nullptr;

// TODO: OOP JIT, enable assert
//Assert(ctorCache == nullptr || srcVal->GetValueInfo()->IsVarConstant() && Js::VarIs<Js::JavascriptFunction>(srcVal->GetValueInfo()->AsVarConstant()->VarValue()));
Assert(ctorCache == nullptr || !ctorCache->IsTypeFinal() || ctorCache->CtorHasNoExplicitReturnValue());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like this is going to produce a guard check where we don't really need one. Is that true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GenCtorObj is performing similar functionality to NewScObjectNoCtor; this change was made because the only instr that can reach this point is GenCtorObj thus isCtorInlined || ctorCache->IsTypeFinal() would always return true.

The way to determine if this bailout is necessary is to see why NewScObjectNoCtor instrs were originally emitted and if the emitted GenCtorObj instr has those same properties.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The key difference is that GenCtorObj is emitted whether the ctor is inlined or not. That isn't true of NewScObjectNoCtor, which is only emitted as the result of inlining (I think). If there isn't an optimization requiring a guard check (usually inlining, but also, apparently, final type opt on the type assigned to the new object), we shouldn't need a guard check. The final type opt seems to belong to GenCtorObj now, since it's creating the object according to the hints in the ctorCache.

if (ctorCache != nullptr && !ctorCache->SkipNewScObject() && (isCtorInlined || ctorCache->IsTypeFinal()))
if (ctorCache != nullptr && !ctorCache->SkipNewScObject())
{
GenerateBailAtOperation(instrPtr, IR::BailOutFailedCtorGuardCheck);
}
Expand All @@ -1616,7 +1616,7 @@ GlobOpt::ValueNumberObjectType(IR::Opnd *dstOpnd, IR::Instr *instr)
return;
}

if (instr->IsNewScObjectInstr())
if (instr->IsNewScObjectInstr()/*|| instr->m_opcode == Js::OpCode::GenCtorObj*/)
{
// If we have a NewScObj* for which we have a valid constructor cache we know what type the created object will have.
// Let's produce the type value accordingly so we don't insert a type check and bailout in the constructor and
Expand All @@ -1628,6 +1628,8 @@ GlobOpt::ValueNumberObjectType(IR::Opnd *dstOpnd, IR::Instr *instr)
Assert(instr->IsProfiledInstr());
Assert(instr->GetBailOutKind() == IR::BailOutFailedCtorGuardCheck);

// TODO: possibly need to consider GenCtorObj for this path as well as
// determine if NewScObj was inlined only using GenCtorObj.
bool isCtorInlined = instr->m_opcode == Js::OpCode::NewScObjectNoCtor;
JITTimeConstructorCache * ctorCache = instr->m_func->GetConstructorCache(static_cast<Js::ProfileId>(instr->AsProfiledInstr()->u.profileId));
Assert(ctorCache != nullptr && (isCtorInlined || ctorCache->IsTypeFinal()));
Expand Down
111 changes: 109 additions & 2 deletions lib/Backend/IR.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -3387,6 +3387,15 @@ bool Instr::CanHaveArgOutChain() const
this->m_opcode == Js::OpCode::NewScObjArraySpread;
}

bool Instr::IsNewScObjCallVariantInstr()
{
return
this->m_opcode == Js::OpCode::NewScObject ||
this->m_opcode == Js::OpCode::NewScObjectSpread ||
this->m_opcode == Js::OpCode::NewScObjArray ||
this->m_opcode == Js::OpCode::NewScObjArraySpread;
}

bool Instr::HasEmptyArgOutChain(IR::Instr** startCallInstrOut)
{
Assert(CanHaveArgOutChain());
Expand All @@ -3408,6 +3417,22 @@ bool Instr::HasEmptyArgOutChain(IR::Instr** startCallInstrOut)
return false;
}

uint Instr::ArgOutChainLength()
{
Assert(CanHaveArgOutChain());

uint length = 0;
Instr* currArgOutInstr = GetSrc2()->GetStackSym()->GetInstrDef();

while (currArgOutInstr->m_opcode != Js::OpCode::StartCall)
{
length++;
currArgOutInstr = currArgOutInstr->GetSrc2()->GetStackSym()->GetInstrDef();
}

return length;
}

bool Instr::HasFixedFunctionAddressTarget() const
{
Assert(
Expand All @@ -3417,14 +3442,96 @@ bool Instr::HasFixedFunctionAddressTarget() const
this->m_opcode == Js::OpCode::NewScObjectSpread ||
this->m_opcode == Js::OpCode::NewScObjArray ||
this->m_opcode == Js::OpCode::NewScObjArraySpread ||
this->m_opcode == Js::OpCode::NewScObjectNoCtor);
this->m_opcode == Js::OpCode::NewScObjectNoCtor ||
this->m_opcode == Js::OpCode::GenCtorObj);
return
this->GetSrc1() != nullptr &&
this->GetSrc1()->IsAddrOpnd() &&
this->GetSrc1()->AsAddrOpnd()->GetAddrOpndKind() == IR::AddrOpndKind::AddrOpndKindDynamicVar &&
this->GetSrc1()->AsAddrOpnd()->m_isFunction;
}

Instr* Instr::GetGenCtorInstr()
{
Assert(IsNewScObjCallVariantInstr());
Instr* currArgOutInstr = this;
Instr* currArgOutInstrValDef = this;
do
{
// TODO: should use helper method here? GetNextInstr?
Assert(currArgOutInstr->GetSrc2());
currArgOutInstr = currArgOutInstr->GetSrc2()->GetStackSym()->GetInstrDef();
Assert(currArgOutInstr);
if (currArgOutInstr->m_opcode == Js::OpCode::GenCtorObj)
{
return currArgOutInstr;
}
if (currArgOutInstr->m_opcode == Js::OpCode::LdSpreadIndices)
{
// This instr is a redirection, move on to next instr.
continue;
}
Assert(currArgOutInstr->m_opcode == Js::OpCode::ArgOut_A);
if (currArgOutInstr->GetSrc1()->IsAddrOpnd())
{
// This instr's src1 is not a symbol, thus it does not have a def instr
// and thus it cannot be from a GenCtorObj instr.
continue;
}

// Looking for the opcode GenCtorObj in currArgOutInstrValDef.
currArgOutInstrValDef = currArgOutInstr->GetSrc1()->GetStackSym()->GetInstrDef();
Assert(currArgOutInstrValDef);
if (currArgOutInstrValDef->m_opcode == Js::OpCode::BytecodeArgOutCapture)
{
if (currArgOutInstrValDef->GetSrc1()->GetStackSym())
{
// Indirection through BytecodeArgOutCapture.
currArgOutInstrValDef = currArgOutInstrValDef->GetSrc1()->GetStackSym()->GetInstrDef();
}
Assert(currArgOutInstrValDef);
}
} while (currArgOutInstrValDef->m_opcode != Js::OpCode::GenCtorObj);
return currArgOutInstrValDef;
}

Opnd* Instr::GetArgOutVal(uint argIndex)
{
Assert(IsNewScObjCallVariantInstr());
Instr* currArgOutInstr = this;
do
{
Assert(currArgOutInstr->GetSrc2());
currArgOutInstr = currArgOutInstr->GetSrc2()->GetStackSym()->GetInstrDef();
Assert(currArgOutInstr);
if (currArgOutInstr->m_opcode == Js::OpCode::StartCall)
{
// argIndex is larger than this argOutChain's length.
return nullptr;
}

if (currArgOutInstr->m_opcode == Js::OpCode::LdSpreadIndices)
{
// This instr is a redirection, move on to next instr.
continue;
}
Assert(currArgOutInstr->m_opcode == Js::OpCode::ArgOut_A);

if (argIndex > 0)
{
argIndex--;
}
} while (argIndex > 0);

Opnd* argOutVal = currArgOutInstr->GetSrc1();
if (argOutVal->GetStackSym()->m_instrDef->m_opcode == Js::OpCode::BytecodeArgOutCapture)
{
argOutVal = argOutVal->GetStackSym()->m_instrDef->GetSrc1();
}

return argOutVal;
}

bool Instr::TransfersSrcValue()
{
// Return whether the instruction transfers a value to the destination.
Expand Down Expand Up @@ -3635,7 +3742,7 @@ uint Instr::GetArgOutCount(bool getInterpreterArgOutCount)
opcode == Js::OpCode::EndCallForPolymorphicInlinee || opcode == Js::OpCode::LoweredStartCall);

Assert(!getInterpreterArgOutCount || opcode == Js::OpCode::StartCall);
uint argOutCount = !this->GetSrc2() || !getInterpreterArgOutCount || m_func->GetJITFunctionBody()->IsAsmJsMode()
uint argOutCount = !this->GetSrc2() || !getInterpreterArgOutCount || m_func->GetJITFunctionBody()->IsAsmJsMode()
? this->GetSrc1()->AsIntConstOpnd()->AsUint32()
: this->GetSrc2()->AsIntConstOpnd()->AsUint32();

Expand Down
10 changes: 9 additions & 1 deletion lib/Backend/IR.h
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,9 @@ class Instr
isCallInstrProtectedByNoProfileBailout(false),
hasSideEffects(false),
isNonFastPathFrameDisplay(false),
isSafeToSpeculate(false)
isSafeToSpeculate(false),
isFixedCall(false),
genCtorInstrHasArgs(false)
#if DBG
, highlight(0)
, m_noLazyHelperAssert(false)
Expand Down Expand Up @@ -357,10 +359,14 @@ class Instr
static IR::Instr * CloneRange(Instr * instrStart, Instr * instrLast, Instr * instrInsert, Lowerer *lowerer, JitArenaAllocator *alloc, bool (*fMapTest)(IR::Instr*), bool clonedInstrGetOrigArgSlot);

bool CanHaveArgOutChain() const;
bool IsNewScObjCallVariantInstr();
bool HasEmptyArgOutChain(IR::Instr** startCallInstrOut = nullptr);
uint Instr::ArgOutChainLength();
bool HasFixedFunctionAddressTarget() const;
// Return whether the instruction transfer value from the src to the dst for copy prop
bool TransfersSrcValue();
Instr* GetGenCtorInstr();
Opnd* GetArgOutVal(uint argIndex);

#if ENABLE_DEBUG_CONFIG_OPTIONS
const char * GetBailOutKindName() const;
Expand Down Expand Up @@ -581,6 +587,8 @@ class Instr
bool isCallInstrProtectedByNoProfileBailout : 1;
bool hasSideEffects : 1; // The instruction cannot be dead stored
bool isNonFastPathFrameDisplay : 1;
bool isFixedCall : 1;
bool genCtorInstrHasArgs : 1;
protected:
bool isCloned : 1;
bool hasBailOutInfo : 1;
Expand Down
24 changes: 12 additions & 12 deletions lib/Backend/IRBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1779,6 +1779,7 @@ IRBuilder::BuildReg2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot R0, Js::Re
case Js::OpCode::SpreadObjectLiteral:
// fall through
case Js::OpCode::SetComputedNameVar:
case Js::OpCode::UpNewScObjCache:
{
IR::Instr *instr = IR::Instr::New(newOpcode, m_func);
instr->SetSrc1(this->BuildSrcOpnd(R0));
Expand All @@ -1805,6 +1806,17 @@ IRBuilder::BuildReg2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot R0, Js::Re
this->AddInstr(instr, offset);
return;
}

// A dest symbol that has a def but no use can be emitted. The symbol can then be reused
// as a dest of another instr. When this occurs the symbol cannot refer to either of the
// two def instrs as its instrDef. In order to ensure that a GenCtorObj instr can be
// referred to using its dest symbol, we force the dest symbol to be a single def. We
// want this property for GenCtorObj because NewScObj* instrs will need to refer to their
// related GenCtorObj instr, this is done through walking up the ArgOut chain and checking
// the ArgOuts' values' instrDefs for the GenCtorObj opcode.
case Js::OpCode::GenCtorObj:
SetTempUsed(R0, TRUE);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason why this isn't the same as case for Js::OpCode::UpNewScObjCache?

Copy link
Contributor

@pleath pleath Jun 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is SetTempUsed accomplishing here? R0 is not a temp and shouldn't be renumbered regardless. (Edit: I see, the passed-in "R0", not the byte code reg "R0".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's odd that we should have to do this. What does the byte code sequence look like? Where is the reg defined?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any input on this question? Thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hack is necessary for the following scenario:

  • A dest symbol that has a def but no use can be emitted (ex: "a = b + c" where "a" is never used)
  • "a" will be given a symbol (ex: "s5") but the next time a dest symbol is required (ex: a GenCtorObj instr) "s5" will be reused since "a" is never used.
  • now "s5" is the dest of two instrs.
  • "s5" loses its instrDef value because it has more than one instrDef

We need instrDef in order to find the GenCtorObj instr during the ArgOut chain traversal. so to solve this issue we make sure that the symbol of a GenCtorObj instr is unique.

This is unnecessary for UpNewScObjCache because UpNewScObjCache is not involved in any ArgOut chain traversal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me a script that exhibits the behavior you're talking about? If an unused dst is not consumed at the def point, we'll have more problems than just the one you're describing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pleath Looks like we ran into this issue for OpCode::StArrInlineItem_CI4 in the past (but handled it differently). Based on comment for that opcode in IRBuilder, I was able to find simple case that doesn't consume the dst: for (let i = 0; i < 0; i + 1) { }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arf. Do you have a little script that behaves this way? We need an automated way of detecting these. Maybe a debug flag that indicates a parse node is participating in a question expression.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think of it, a hint from the parser to identify intermediate results of ?: ops might allow us to simplify all these things and eliminate Unused altogether. That's probably the way to go, not least because there's nothing that requires Unused for correctness, so we'll keep getting these unrenamed lifetimes. I'll look into that.

break;
}

IR::RegOpnd * dstOpnd = this->BuildDstOpnd(R0);
Expand All @@ -1820,7 +1832,6 @@ IRBuilder::BuildReg2(Js::OpCode newOpcode, uint32 offset, Js::RegSlot R0, Js::Re
dstSym->m_builtInIndex = symSrc1->m_builtInIndex;
}
break;

case Js::OpCode::ProfiledStrictLdThis:
newOpcode = Js::OpCode::StrictLdThis;
if (m_func->HasProfileInfo())
Expand Down Expand Up @@ -6536,7 +6547,6 @@ IRBuilder::BuildCallI_Helper(Js::OpCode newOpcode, uint32 offset, Js::RegSlot ds
case Js::OpCode::NewScObjArray:
case Js::OpCode::NewScObjArraySpread:
symDst->m_isSafeThis = true;
symDst->m_isNotNumber = true;
break;
}
}
Expand All @@ -6551,7 +6561,6 @@ IRBuilder::BuildCallI_Helper(Js::OpCode newOpcode, uint32 offset, Js::RegSlot ds
void
IRBuilder::BuildCallCommon(IR::Instr * instr, StackSym * symDst, Js::ArgSlot argCount, Js::CallFlags flags)
{
Js::OpCode newOpcode = instr->m_opcode;

IR::Instr * argInstr = nullptr;
IR::Instr * prevInstr = instr;
Expand All @@ -6578,15 +6587,6 @@ IRBuilder::BuildCallCommon(IR::Instr * instr, StackSym * symDst, Js::ArgSlot arg
this->callTreeHasSomeProfileInfo = false;
}

if (newOpcode == Js::OpCode::NewScObject || newOpcode == Js::OpCode::NewScObjArray
|| newOpcode == Js::OpCode::NewScObjectSpread || newOpcode == Js::OpCode::NewScObjArraySpread)
{
#if DBG
count++;
#endif
m_argsOnStack++;
}

argCount = Js::CallInfo::GetArgCountWithExtraArgs(flags, argCount);

if (argInstr)
Expand Down
Loading