1461888 - Remove the offset for 'trueEnd' from SRC_IF_ELSE and SRC_COND.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Tooru Fujisawa [:arai]]

https://searchfox.org/mozilla-central/rev/00dd116638001772fe354b081353b73f1cad405d/js/src/frontend/BytecodeEmitter.cpp#2096-2105
> class MOZ_STACK_CLASS IfThenElseEmitter
> {
> ...
>     bool emitElse() {
> ...
>         // Annotate SRC_IF_ELSE or SRC_COND with the offset from branch to
>         // jump, for IonMonkey's benefit.  We can't just "back up" from the pc
>         // of the else clause, because we don't know whether an extended
>         // jump was required to leap from the end of the then clause over
>         // the else clause.
>         if (!bce_->setSrcNoteOffset(noteIndex_, 0,
>                                     jumpsAroundElse_.offset - jumpAroundThen_.offset))
>         {
>             return false;
>         }

it says the offset is necessary and it cannot be calculated from 'elseStart' (the jump offset of JSOP_IFEQ),
but it's not unclear why.
(I'm not sure what "extended jump" means here...)

it would be better investigate, and if it's really necessary, it would be nice to add example in the comment that describes how it cannot, with some figure clarifies the difference between 'elseStart' and 'trueEnd'.

Comment 1

[Tooru Fujisawa [:arai]]

s/elseStart/falseStart/g

Comment 2

[Tooru Fujisawa [:arai]]

here's try run which calculates 'trueEnd' from 'falseStart - JSOP_GOTO_LENGTH', and asserts that it's same as source note's 'trueEnd'.
I think this should be true since now we control the bytecode sequence only in IfThenElseEmitter, and we don't emit otherwise.

If the try run finishes without failure, I'll remove the offset from source notes.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1059c197ae02049b9715e329bc6619bf04382229

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Remove trueEnd offset from SRC_IF_ELSE and SRC_COND.

also green on try run with the offset removed from the source note.
https://hg.mozilla.org/try/rev/80ff5c465604b146f0538765d92b5fcd900797d8

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 8976757 [details] [diff] [review]
Remove trueEnd offset from SRC_IF_ELSE and SRC_COND.

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

Nice!

::: js/src/jit/IonControlFlow.cpp
@@ +1775,2 @@
>      //    ...
> +    // Z: JUMPTARGET ; join

Pre-existing: this should be s/Z:/X:/ I think

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #0)
> (I'm not sure what "extended jump" means here...)

It used to be that bytecode had different jump opcodes/sizes (extended/bigger jump instructions for "far" branches), so that's why we needed the source note. The comment is saying we couldn't do "falseStart - JSOP_GOTO_LENGTH" because the length of the instruction depended on the goto kind.

Comment 6

[Tooru Fujisawa [:arai]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/01d9634bf82ef07002dd46be45e11ffc677a4167
Bug 1461888 - Remove trueEnd offset from SRC_IF_ELSE and SRC_COND. r=jandem

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/01d9634bf82e