980263 - Fix Ion compilation of functions with loops nested in expressions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Andy Wingo [:wingo]]

Before we were able to Ion-compile "let" scopes, we never saw a case in which Ion could try to compile a function with a loop nested in an expression.  Now we do see those cases; see bug 956166 for an interesting crasher.

The root of the problem is that a lot of Ion code that processes stack expressions expects operands to be values and not phis.  Indeed we never see a phi for many kinds of stack expressions, besides ternary expressions.  However, Ion currently places phis at all values on the stack at a loop, which can invalidate those invariants.

There are two things that need to be done in order to enable Ion to work (or at least not to crash) on this kind of code.

The first  part is to disable OSR for loops that have non-loop state on the stack, because OSR always introduces phis for such values.

The second is to avoid creating phis for non-loop state on the stack, because such values have only one definition anyway.

Comment 1

[Andy Wingo [:wingo]]

Attachment: Part 1: Disable Ion OSR for loops nested in expressions

Comment 2

[Andy Wingo [:wingo]]

Attachment: Part 2: Avoid phi creation for values on stack at loops

Comment 3

[Andy Wingo [:wingo]]

This part-2 patch does fix the test case from bug 956166, but it fails in one jit-test, basic/testTypedArrayInit.js, with the error:

Catchpoint 1 (signal SIGSEGV), 0x00000000007e09f1 in js::jit::LiveRangeAllocator<js::jit::LinearScanVirtualRegister, true>::buildLivenessInfo (this=0x7fffffffba40)
    at /hack/mozilla-central/js/src/jit/LiveRangeAllocator.cpp:832
832	        JS_ASSERT_IF(!mblock->numPredecessors(), live->empty());
(gdb) bt
#0  0x00000000007e09f1 in js::jit::LiveRangeAllocator<js::jit::LinearScanVirtualRegister, true>::buildLivenessInfo (this=0x7fffffffba40) at /hack/mozilla-central/js/src/jit/LiveRangeAllocator.cpp:832
#1  0x00000000007229c2 in js::jit::LinearScanAllocator::go (this=0x7fffffffba40) at /hack/mozilla-central/js/src/jit/LinearScan.cpp:1238
#2  0x00000000006c9e69 in js::jit::GenerateLIR (mir=0x1c95f60) at /hack/mozilla-central/js/src/jit/Ion.cpp:1428
#3  0x00000000006ca1cc in js::jit::CompileBackEnd (mir=0x1c95f60, maybeMasm=0x0) at /hack/mozilla-central/js/src/jit/Ion.cpp:1519
#4  0x00000000006caf6f in js::jit::IonCompile (cx=0x1ace7a0, script=0x7ffff584a1f0, baselineFrame=0x7fffffffc6d0, osrPc=0x1bace71 "\343\203V", constructing=false, executionMode=js::SequentialExecution, 
    recompile=false, optimizationLevel=js::jit::Optimization_Normal) at /hack/mozilla-central/js/src/jit/Ion.cpp:1768
#5  0x00000000006cba7c in js::jit::Compile (cx=0x1ace7a0, script=0x7ffff584a1f0, osrFrame=0x7fffffffc6d0, osrPc=0x1bace71 "\343\203V", constructing=false, executionMode=js::SequentialExecution)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:1971
#6  0x00000000006cbd3e in js::jit::CanEnterAtBranch (cx=0x1ace7a0, script=0x7ffff584a1f0, osrFrame=0x7fffffffc6d0, pc=0x1bace71 "\343\203V", isConstructing=false)
    at /hack/mozilla-central/js/src/jit/Ion.cpp:2033
#7  0x00000000005ecc23 in js::jit::EnsureCanEnterIon (cx=0x1ace7a0, stub=0x1bb9b50, frame=0x7fffffffc6d0, script=0x7ffff584a1f0, pc=0x1bace71 "\343\203V", jitcodePtr=0x7fffffffc5b8)
    at /hack/mozilla-central/js/src/jit/BaselineIC.cpp:770
#8  0x00000000005ed3f1 in js::jit::DoUseCountFallback (cx=0x1ace7a0, stub=0x1bb9b50, frame=0x7fffffffc6d0, infoPtr=0x7fffffffc668) at /hack/mozilla-central/js/src/jit/BaselineIC.cpp:941

Not sure what's going on here.

Comment 4

[Andy Wingo [:wingo]]

Attachment: Part 2: Avoid phi creation for values on stack at loops

Comment 5

[Andy Wingo [:wingo]]

https://tbpl.mozilla.org/?tree=Try&rev=68c784f8fcd9

Comment 6

[Andy Wingo [:wingo]]

Comment on attachment 8386829 [details] [diff] [review]
Part 2: Avoid phi creation for values on stack at loops

Updated patch adds phis for all stack values at all potential OSR positions.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8386686 [details] [diff] [review]
Part 1: Disable Ion OSR for loops nested in expressions

Review of attachment 8386686 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great! r=me with nits below addressed.

::: js/src/frontend/BytecodeEmitter.cpp
@@ +84,5 @@
>  
> +
> +namespace {
> +
> +struct LoopStmtInfo : public StmtInfoBCE {

Nit: { on its own line.

@@ +89,5 @@
> +    int32_t         stackDepth;     // Stack depth when this loop was pushed.
> +    uint32_t        loopDepth;      // Loop depth.
> +
> +    // Can we OSR here?  True unless there is non-loop state on the stack.
> +    bool            canOsr;

Nit: s/canOsr/canIonOsr

@@ +668,5 @@
>  #define SET_STATEMENT_TOP(stmt, top)                                          \
>      ((stmt)->update = (top), (stmt)->breaks = (stmt)->continues = (-1))
>  
>  static void
> +PushStatementInner(BytecodeEmitter *bce, StmtInfoBCE *stmt, StmtType type, ptrdiff_t top) 

Nit: trailing whitespace.

::: js/src/jsopcode.h
@@ +208,5 @@
>  #define LOCALNO_BITS            24
>  #define LOCALNO_LIMIT           (1 << LOCALNO_BITS)
>  
> +static inline unsigned
> +LoopEntryDepthHint(jsbytecode *pc) {

Style nit: { on its own line, also for the functions below.

@@ +213,5 @@
> +    JS_ASSERT(*pc == JSOP_LOOPENTRY);
> +    return GET_UINT8(pc) & 0x7f;
> +}
> +static inline bool
> +LoopEntryCanOsr(jsbytecode *pc) {

Please rename this to LoopEntryCanIonOsr, as interpreter -> Baseline OSR is still possible.

::: js/src/vm/Opcodes.h
@@ +477,5 @@
>      macro(JSOP_IMPLICITTHIS,  226, "implicitthis", "",    5,  0,  1,  JOF_ATOM) \
>      \
>      /*
> +     * This opcode is the target of the entry jump for some loop. The uint8
> +     * argument is a bitfield. The lower 7 bits of the argument indicate the

We should bump XDR_BYTECODE_VERSION in Xdr.h

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8386829 [details] [diff] [review]
Part 2: Avoid phi creation for values on stack at loops

Review of attachment 8386829 [details] [diff] [review]:
-----------------------------------------------------------------

Perfect.

::: js/src/jit/MIRGraph.h
@@ +46,5 @@
>      MBasicBlock(MIRGraph &graph, CompileInfo &info, jsbytecode *pc, Kind kind);
>      bool init();
>      void copySlots(MBasicBlock *from);
> +    bool inherit(TempAllocator &alloc, BytecodeAnalysis *analysis, MBasicBlock *pred,
> +                 uint32_t popped, unsigned loopStateSlots = 0);

Uber nit: s/loopStateSlots/stackPhiCount here to match the .cpp file (also below).

Comment 9

[Andy Wingo [:wingo]]

Thanks for the review :)

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f0e946c9674
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9fc2eb18bd1

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f0e946c9674
https://hg.mozilla.org/mozilla-central/rev/b9fc2eb18bd1