Enable jitting generators by default

[nhat-nguyen]

No description provided.

[MikeHolman]

I think the flag is still useful, so could we use CONFIG_FLAG macro and change the default in configflagslist.h to true instead of removing it altogether? (difference between macros is that CONFIG_ISENABLED is always false on release build, where CONFIG_FLAG takes the default value)

[rhuanjl]

Adding the below on line 1776 of BailOut.cpp removes the crash with AsyncGenerators that this PR is currently hitting:

    if (executeFunction->IsCoroutine() && bailOutKind != IR::BailOutForGeneratorYield)
    {
        Js::ResumeYieldData* resumeYieldData = static_cast<Js::ResumeYieldData*>(args[1]);
        newInstance->SetNonVarReg(executeFunction->GetYieldRegister(), resumeYieldData);
    }

Should I open a new PR with the content of this one plus the fix, or leave for someone else to pick and incorporate?

[rhuanjl]

I'm still hoping to enable jitting generators but still have a key bug that's stopping me from doing so - I know it's been a while but please can someone who's looked at it before see if they have any suggestions?

Problem comes when you have an unconditional bailout in a jitted generator (e.g. a BailOnNoProfile).

In the DeadStore phase any code after that BailOnNoProfile can get deadstored - when code has been deadstored syms relating to it seem to be missed in generating the BailIn.

Example: (run with -jitEs6Generators -mic:1 -off:simplejit -trace:bailout)

function* gf()
{
  for (let i = 0; i < 3; ++i)
  {
    yield i;
  }
}
const gen = gf();

print(gen.next().value);
print(gen.next().value);
print(gen.next().value);

Result:

BailOut: function: gf ( (#1.1), #2) offset: #001e Opcode: Yield Kind: BailOutForGeneratorYield
0
BailOut: function: gf ( (#1.1), #2) offset: #0021 Opcode: BailOnNoProfile Kind: BailOutOnNoProfile
1
BailOut: function: gf ( (#1.1), #2) offset: #0021 Opcode: BailOnNoProfile Kind: BailOutOnNoProfile
1

Steps of what has happened:

1. yield 0 (i is still 0)
2. bail-in (at the bottom of the loop) - immediately hit bail on no profile
3. in interpreter increment i and then yield
4. bail-in (in the correct place BUT don't restore i - as the increment was deadstored)
5. bail on no profile (again) - at this point i is now 0 when it should be 1
6. in interpreter increment i and yield (the wrong value)

[nhat-nguyen]

@rhuanjl The bail-in code should work even with BailOnNoProfile (I vaguely remember having to debug some bugs similar to this); however, there were many cases that I had to handle differently, so it is likely that I might have missed something.

The code that builds the bail-in symbol list is here:

ChakraCore/lib/Backend/LinearScan.cpp

Line 5072 in 11d9375

void LinearScan::GeneratorBailIn::BuildBailInSymbolList(

2 functions that determine whether a symbol needs to be reloaded when bailing in:

ChakraCore/lib/Backend/LinearScan.cpp

Line 5312 in 11d9375

bool LinearScan::GeneratorBailIn::NeedsReloadingSymWhenBailingIn(StackSym* sym) const

ChakraCore/lib/Backend/LinearScan.cpp

Line 5300 in 11d9375

bool LinearScan::GeneratorBailIn::NeedsReloadingBackendSymWhenBailingIn(StackSym* sym) const

I would start with checking whether either of the two functions correctly determines if the symbol needs reloading. It's been almost 2 years since I last worked on Chakra, so I can't remember any specific implementation; but hopefully this can help you narrow down the issue.

#6183 can also be helpful when debugging bail-in code. :)

[pleath]

The usual mechanism for indicating that a byte code register will be live on bailout at a point in the code where it has been optimized away is the ByteCodeUses instruction. Are you familiar with how those are generated?

[rhuanjl]

Thank you so much for the pointers @nhat-nguyen and @pleath - in the cases I'm looking at ByteCodeUses instructions are inserted for the relevant regs AND they go from jit -> interpreter when bailing out BUT they don't get loaded back when bailing in. I think with my example loop above the loop control variable gets marked as singledef when there's a BailOnNoProfile in the loop which results in the BailIn code dropping it - I'll keep digging.

Also unfortunately I've found a second bug that is rather strange, if you have any thoughts I'd really appreciate it:

This code crashes (AV trying to load the for-in enumerator) if it runs from the start in the JIT, but not if it does its startup yield in the interpreter.

function* gf(a)
{
  for (let i in arr)
  {
    for (let j in arr2)
    {
      yield i + j;
    }
  }
}

However this code runs fine:

function* gf(a)
{
  print("running");
  for (let i in arr)
  {
    for (let j in arr2)
    {
      yield i + j;
    }
  }
}

The only relevant difference I can see is that in the RegAlloc phase, in the crashing case this:

    s24.u64         =  MOV            [s23.u64!+32 <Js::JavascriptGenerator::GetFrameOffset().GeneratorFrame>].var
    s29.u64         =  MOV            [s24.u64+112 <Js::InterpreterStackFrame::GetOffsetOfForInEnumerators().ForInEnumerators>].var
    s25.u64         =  MOV            [s24.u64+8 <Js::InterpreterStackFrame::GetCurrentLocationOffset().InterpreterCurrentLocation>].var
    s26.u64         =  MOV            [s24.u64! <Js::InterpreterStackFrame::GetStartLocationOffset().InterpreterStartLocation>].var

Turns into this:

    s24(rax).u64    =  MOV            [s23(rax).u64!+32 <Js::JavascriptGenerator::GetFrameOffset().GeneratorFrame>].var
    s29(rcx).u64    =  MOV            [s24(rax).u64+112 <Js::InterpreterStackFrame::GetOffsetOfForInEnumerators().ForInEnumerators>].var
    s29<-208>.u64   =  MOV            s29(rcx).u64
    s25(rdx).u64    =  MOV            [s24(rax).u64+8 <Js::InterpreterStackFrame::GetCurrentLocationOffset().InterpreterCurrentLocation>].var
    s26(rax).u64    =  MOV            [s24(rax).u64! <Js::InterpreterStackFrame::GetStartLocationOffset().InterpreterStartLocation>].var

[nhat-nguyen]

@rhuanjl Since it only crashes in the jit the very first time when loading generator for-in, I think it may be because the generator frame might have not been initialized correctly. Perhaps you can take a look at:

ChakraCore/lib/Backend/IRBuilder.cpp

Line 7911 in dd0d9ec

IRBuilder::GeneratorJumpTable::BuildJumpTable()

[rhuanjl]

@rhuanjl Since it only crashes in the jit the very first time when loading generator for-in, I think it may be because the generator frame might have not been initialized correctly. Perhaps you can take a look at:

ChakraCore/lib/Backend/IRBuilder.cpp

Line 7911 in dd0d9ec

IRBuilder::GeneratorJumpTable::BuildJumpTable()

I had considered this - and I’ve tried pulling the frame initialisation code out of the jitted code and into JavascriptGenerator::CallGenerator. Which I think makes the logic cleaner but didn’t solve this problem.

Notably this issue occurs only if these 3 factors are combined:

1. A nested for-in loop (one for-in is fine problem is only with nesting)
2. No other code above the for-in loop at the top of the function - adding a seemingly irrelevant print cal at the top of the function “fixes” the error
3. First iteration of the loop (and hence for-in enumerator initialisation) happens in the jit.

[rhuanjl]

Thanks for the tips - made some progress; I think I've fixed the for-in issue, but still not solved the BailOnNoProfile issue.

Though - I think I've worked out the issue, using verbose bailouts the loop control var is shown as: "BailOut: Register # 7: Constant index 0, value: 0x0001000000000000"

I think the increment operation has been killed as it's after the BailOnNoProfile and so the jit has concluded that the control var is a const. I assume somewhere there is a specific optimisation I need to disable in loops containing a yield - I've spent some time hunting but can't find it.

[rhuanjl]

Superseded by #6662

Thank you for all the help.