Remove BytecodeAnalysis from IonBuilder

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

(Blocks: 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(8 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
Created attachment 8888690 [details] [diff] [review]
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.)
Attachment #8888690 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 2

a year ago
Created attachment 8888692 [details] [diff] [review]
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.
Attachment #8888692 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 3

a year ago
Created attachment 8888693 [details] [diff] [review]
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.
Attachment #8888693 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 4

a year ago
Created attachment 8888697 [details] [diff] [review]
Part 4 - Cache usesEnvironmentChain flag on the BaselineScript

We can cache analysis.usesEnvironmentChain() on the BaselineScript so IonBuilder can use it.
Attachment #8888697 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 5

a year ago
Created attachment 8888708 [details] [diff] [review]
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).
Attachment #8888708 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 6

a year ago
Created attachment 8888715 [details] [diff] [review]
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).
Attachment #8888715 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 7

a year ago
Created attachment 8888716 [details] [diff] [review]
Part 6 - Remove BytecodeAnalysis::hasTryFinally
Attachment #8888715 - Attachment is obsolete: true
Attachment #8888715 - Flags: review?(nicolas.b.pierron)
Attachment #8888716 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 8

a year ago
Created attachment 8888721 [details] [diff] [review]
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.
Attachment #8888721 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 9

a year ago
Created attachment 8888724 [details] [diff] [review]
Part 8 - Remove BytecodeAnalysis from IonBuilder and ControlFlowGenerator

We can also remove the GSNCache in IonBuilder.
Attachment #8888724 - Flags: review?(nicolas.b.pierron)
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.
Attachment #8888690 - Flags: review?(nicolas.b.pierron)
(Assignee)

Comment 11

a year ago
(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.
Flags: needinfo?(nicolas.b.pierron)
Attachment #8888692 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8888690 - Flags: review+
Attachment #8888693 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8888697 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8888708 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8888716 - Flags: review?(nicolas.b.pierron) → review+
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
Attachment #8888721 - Flags: review?(nicolas.b.pierron) → review+
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.
Attachment #8888724 - Flags: review?(nicolas.b.pierron) → review+
(Assignee)

Comment 15

a year ago
(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

a year ago
Pushed by jandemooij@gmail.com:
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
(Assignee)

Updated

a year ago
Flags: needinfo?(nicolas.b.pierron)
(Assignee)

Updated

a year ago
No longer depends on: 1383972
You need to log in before you can comment on or make changes to this bug.