1382973 - Remove BytecodeAnalysis from IonBuilder

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Jan de Mooij [:jandem]]

Both Baseline and Ion compilation currently require running the BytecodeAnalysis step on each script to determine stack depths and some other bits. The analysis is pretty fast but it does show up in profiles and it allocates quite a lot of LifoAlloc memory.

I have some patches to remove this analysis from IonBuilder as we can use other mechanisms to get this data.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Don't use BytecodeAnalysis for stack depths

This rewrites the MBasicBlock::New functions etc to take the stack depth instead of the BytecodeAnalysis*.

(Before I removed the BytecodeAnalysis* arguments, I first asserted we got the same values as before and ran all shell tests, so things should be the same as before.)

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Get rid of maybeInfo checks

analyzeNewLoopTypes uses the BytecodeAnalysis to skip ops that are not reachable. We can just remove that check if we teach some BaselineInspector code to deal with (unreachable) ops without an ICEntry.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - Add IonBuilder::usesEnvironmentChain

This is just a simple refactoring to add IonBuilder::usesEnvironmentChain so we don't have to use analysis().usesEnvironmentChain() everywhere.

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Cache usesEnvironmentChain flag on the BaselineScript

We can cache analysis.usesEnvironmentChain() on the BaselineScript so IonBuilder can use it.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Remove BytecodeAnalysis::hasSetArg

This one is easy because baselineScript->modifiesArguments() is exactly the same thing (set to true when the script has a JSOP_SETARG).

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove BytecodeAnalysis::hasTryFinally

For hasTryFinally I think it's best to just check for JSTRY_FINALLY try notes when we see a JSOP_TRY in Ion. We check this at most once per script. There are other options but I think this is the simplest one (mostly because we don't have a BaselineScript when we run the arguments analysis).

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 6 - Remove BytecodeAnalysis::hasTryFinally

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 7 - Simplify try-catch code

At this point the BytecodeAnalysis is only used to check for unreachable code after a try-catch block. This edge case is responsible for quite a lot of complexity and I think it's simpler to teach the BytecodeAnalysis to always mark the code after a try-catch as reachable.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 8 - Remove BytecodeAnalysis from IonBuilder and ControlFlowGenerator

We can also remove the GSNCache in IonBuilder.

Comment 10

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8888690 [details] [diff] [review]
Part 1 - Don't use BytecodeAnalysis for stack depths

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

::: js/src/jit/IonBuilder.cpp
@@ +3842,5 @@
>  
>      // Create return block.
>      jsbytecode* postCall = GetNextPc(pc);
>      MBasicBlock* returnBlock;
> +    MOZ_TRY_VAR(returnBlock, newBlock(current->stackDepth(), postCall));

This line sounds wrong, even in the existing code because:
 - postCall is part of the caller.
 - this return block is being given a caller resume point.

I think It would make much more sense to use the postCall pc, remove the caller resume point (below this line), and use the same stack depth as done in IonBuilder::InlineCalls, i-e  "dispatchBlock->stackDepth() - callInfo.numFormals() + 1".

@@ +4538,5 @@
>      } else {
>          dispatch = MFunctionDispatch::New(alloc(), callInfo.fun());
>      }
>  
> +    MOZ_ASSERT(dispatchBlock->stackDepth() >= callInfo.numFormals() + 1);

nit: MOZ_ASSERT(dispatchBlock->stackDepth() >= callInfo.numFormals());

Otherwise I am not sure to understand this +1, which I associated to the returned value Phi.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #10)
> This line sounds wrong, even in the existing code because:
>  - postCall is part of the caller.
>  - this return block is being given a caller resume point.
> 
> I think It would make much more sense to use the postCall pc, remove the
> caller resume point (below this line), and use the same stack depth as done
> in IonBuilder::InlineCalls, i-e  "dispatchBlock->stackDepth() -
> callInfo.numFormals() + 1".

There are no arguments on the stack at this point. Also note that right after this we do:

    // Inherit the slots from current and pop |fun|.
    returnBlock->inheritSlots(current);
    returnBlock->pop();

inheritSlots will also set returnBlock's stackDepth to current->stackDepth() and then we pop() the function, so it doesn't matter much what we pass to newBlock.

I'm happy to make follow-up changes but I'd rather not change behavior (like removing resume points) in this patch, to avoid regressions.

> > +    MOZ_ASSERT(dispatchBlock->stackDepth() >= callInfo.numFormals() + 1);
> 
> nit: MOZ_ASSERT(dispatchBlock->stackDepth() >= callInfo.numFormals());
> 
> Otherwise I am not sure to understand this +1, which I associated to the
> returned value Phi.

Ok I can remove this +1.

Comment 12

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8888690 [details] [diff] [review]
Part 1 - Don't use BytecodeAnalysis for stack depths

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

Oh, nevermind, I did not pay attaention that current was from the caller builder and not from the callee builder.  The caller resume point is the same, so forget about my last comment.

Comment 13

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8888721 [details] [diff] [review]
Part 7 - Simplify try-catch code

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

::: js/src/jit/BytecodeAnalysis.cpp
@@ +140,5 @@
> +            // Ensure the code following the try-block is always marked as
> +            // reachable, to simplify Ion's ControlFlowGenerator.
> +            uint32_t afterTryOffset = script_->pcToOffset(afterTry);
> +            infos_[afterTryOffset].init(stackDepth);
> +            infos_[afterTryOffset].jumpTarget = true;

note: Why do we need a jumpTarget field in the BytecodeInfo structure?  This is supposed to be statically known from the bytecode now, with the BytecodeIsJumpTarget function[1], and verified by the JSScript::assertValidJumpTarget function[2].

[1] http://searchfox.org/mozilla-central/source/js/src/jsopcode.h#439
[2] http://searchfox.org/mozilla-central/source/js/src/jsscript.cpp#3054

Comment 14

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8888724 [details] [diff] [review]
Part 8 - Remove BytecodeAnalysis from IonBuilder and ControlFlowGenerator

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

Sweet! Removing the source notes from IonBuilder is an awesome step forward unifying all the bytecode iterators.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Nicolas B. Pierron [:nbp] from comment #13)
> note: Why do we need a jumpTarget field in the BytecodeInfo structure?  This
> is supposed to be statically known from the bytecode now, with the
> BytecodeIsJumpTarget function[1], and verified by the
> JSScript::assertValidJumpTarget function[2].

Yeah I agree it would be nice to remove this. For the stack depths it would be cool if the bytecode stored the depth explicitly for certain jump targets (maybe as JSOP_JUMPTARGET operand). Then we could merge BytecodeAnalysis with BaselineCompiler so we have to do a single bytecode iteration instead of two :)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c27fa0fef406
part 1 - Don't use BytecodeAnalysis to compute stack depths in Ion. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/08965781acb1
part 2 - Eliminate some BytecodeAnalysis::maybeInfo checks in IonBuilder. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd75c61ada0
part 3 - Add IonBuilder::usesEnvironmentChain() helper function. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/2126f7e82cc9
part 4 - Cache the BytecodeAnalysis usesEnvironmentChain flag on the BaselineScript. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/99d8af9fffb4
part 5 - Remove BytecodeAnalysis::hasSetArg(). r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd2c01041ec8
part 6 - Remove BytecodeAnalysis::hasTryFinally(). r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfc33f7e1de4
part 7 - Change BytecodeAnalysis to mark code after try-catch as reachable to simplify Ion code. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5acec377801
part 8 - Remove BytecodeAnalysis from IonBuilder and ControlFlowGenerator. r=nbp

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c27fa0fef406
https://hg.mozilla.org/mozilla-central/rev/08965781acb1
https://hg.mozilla.org/mozilla-central/rev/2cd75c61ada0
https://hg.mozilla.org/mozilla-central/rev/2126f7e82cc9
https://hg.mozilla.org/mozilla-central/rev/99d8af9fffb4
https://hg.mozilla.org/mozilla-central/rev/dd2c01041ec8
https://hg.mozilla.org/mozilla-central/rev/bfc33f7e1de4
https://hg.mozilla.org/mozilla-central/rev/f5acec377801