NewScObj refactor #6180
NewScObj refactor #6180
Conversation
wyrichte
force-pushed
the
build/wyrichte/NewScObj/BASE_squash_rebase_bcup_refactor_squash
branch
2 times, most recently
from
b3cacab
to
9ae76ce
Compare
|
Have you done perf testing? |
| // up the ArgOut chain, this instr is accessible via instrDef and thus this instr's | ||
| // dst cannot have a symbol that has already been defined. | ||
| case Js::OpCode::GenCtorObj: | ||
| SetTempUsed(R0, TRUE); |
There was a problem hiding this comment.
any reason why this isn't the same as case for Js::OpCode::UpNewScObjCache?
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
It's odd that we should have to do this. What does the byte code sequence look like? Where is the reg defined?
There was a problem hiding this comment.
Do you have any input on this question? Thanks.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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) { }
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| { | ||
| 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()); | ||
|
|
There was a problem hiding this comment.
It seems like this is going to produce a guard check where we don't really need one. Is that true?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
…stinct parts: GenCtorObj (makes ctor object), NewScObj (calls ctor with ctor object), determine ret val (done using multiple bytecode instrs), UpdateNewScObjCache.
wyrichte
force-pushed
the
build/wyrichte/NewScObj/BASE_squash_rebase_bcup_refactor_squash
branch
from
17d30e5
to
3a216cb
Compare
NewScObj refactor - Splits NewScObj into multiple bytecodes with 4 distinct parts: GenCtorObj (makes ctor object), NewScObj (calls ctor with ctor object), determine ret val (done using multiple bytecode instrs), UpdateNewScObjCache.