Disable OpHelperReg optimisation for Coroutines #6706
Conversation
| const BVSparse<JitArenaAllocator>& byteCodeUpwardExposedUses, | ||
| const BVSparse<JitArenaAllocator>& upwardExposedUses, | ||
| const CapturedValues& capturedValues, |
There was a problem hiding this comment.
These parameters weren't used
| bailInInstr->capturedValues, | ||
| insertionPoint | ||
| ); | ||
| Assert(!this->func->IsStackArgsEnabled()); |
There was a problem hiding this comment.
This Assert had gone adrift from the comment that explained it - therefore moved upwards.
lib/Backend/LinearScan.cpp
Outdated
| @@ -4974,16 +4968,6 @@ LinearScan::GeneratorBailIn::~GeneratorBailIn() | |||
| JitAdelete(this->func->m_alloc, this->bailInSymbols); | |||
| } | |||
|
|
|||
| void LinearScan::GeneratorBailIn::SpillRegsForBailIn() | |||
There was a problem hiding this comment.
This function was called from inside the always false condition above. Aside from introducing segfaults, analysis of the logic shows it's unnecessary.
The purpose of this function was to free RAX and RCX (or EAX and ECX) before a GeneratorBailIn as the logic will use those two registers - BUT - the way the BailIn code works it restores whatever was in RAX and RCX before the Yield/Await as its last step, so freeing them serves no purpose.
| #ifdef DBG | ||
| else | ||
| { | ||
| labelInstr->AsLabelInstr()->m_noHelperAssert = true; |
There was a problem hiding this comment.
There's an assertion to check that helper blocks are being set correctly - with this optimisation disabled it would fire incorrectly, setting this debug only property disables it.
rhuanjl
force-pushed
the
gen_scope_slots
branch
from
64c3d78
to
d3ce892
Compare
rhuanjl
force-pushed
the
gen_scope_slots
branch
2 times, most recently
from
1e4632d
to
28968e1
Compare
rhuanjl
force-pushed
the
gen_scope_slots
branch
from
28968e1
to
af1b538
Compare
| @@ -3339,7 +3339,7 @@ LinearScan::KillImplicitRegs(IR::Instr *instr) | |||
| return; | |||
| } | |||
|
|
|||
| if (instr->m_opcode == Js::OpCode::Yield) | |||
| if (instr->m_opcode == Js::OpCode::GeneratorBailInLabel) | |||
There was a problem hiding this comment.
The OpCode Yield is removed earlier in the jitting process and replaced with a number of operations - the one we're looking for here is the GeneratorBailInLabel.
|
I thought I had this one... back to the drawing board. (The x64 vs x86 differences on this case are a total pain). |
The book keeping for the OpHelperReg optimisation does not always work across yield/await points resulting in potential undefined behaviour.
Disable this optimisation within Coroutines (Generator, Async and AsyncGenerator functions) until/unless a fix can be found.
Additionally the code to spill RAX + RCX/EAX + ECX before GeneratorBailIn was broken - fix it (wrong condition check).
And a little tidy up/removing unused parameters.
Fix: #6704
Fix: #6691