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

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P3
minor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: arai, Assigned: arai)

Tracking

(Blocks 1 bug)

Trunk
mozilla62
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox62 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

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

Comment 1

a year ago
s/elseStart/falseStart/g
Assignee

Comment 2

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

Comment 3

a year ago
also green on try run with the offset removed from the source note.
https://hg.mozilla.org/try/rev/80ff5c465604b146f0538765d92b5fcd900797d8
Attachment #8976757 - Flags: review?(jdemooij)
Assignee

Updated

a year ago
Summary: Do SRC_IF_ELSE and SRC_COND really need offset for 'trueEnd' ? → Remove the offset for 'trueEnd' from SRC_IF_ELSE and SRC_COND.
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
Attachment #8976757 - Flags: review?(jdemooij) → review+
(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.
Priority: -- → P3

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/01d9634bf82e
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.