Closed Bug 1072188 Opened 10 years ago Closed 10 years ago

Range Analysis: Truncate should clone & recover instructions which have removed uses.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: nbp, Assigned: nbp)

References

Details

Attachments

(3 files)

Range Analysis is a destructive optimization, and as such it is unable to truncate instructions which have removed uses.  The reason being that if we ever fail, then we have to resume the execution with a result which does not correspond to the expected result.

As such, any mechanism used for removing a branch, such as UCE, will prevent optimizations made by Range Analysis.  The following code should illustrate this issue quite well.


var uceFault = function (i) {
    if (i > 98)
        uceFault = function (i) { return true; };
    return false;
}

function rbitnot_number(i) {
    var x = (i | 0) * (i >> 16);
    if (uceFault(i) || uceFault(i))
        print(x);
    return x | 0;
}


Note that in the previous test case, the "print(x)" is not needed, but it illustrates what might be done in a branch which is removed.

This bug is made to add use recover instruction, when removed uses are only captured by recoverable operands of the resume points.  This implies that any instruction which can lead to a different result has to be cloned, and then truncated.  The clone is then marked as being recovered on bailout.  So if we unexpectedly enter a removed branch (via a bailout), then the non-truncated result can be computed, even if we compute the truncated one in the generated code.  i-e:


function rbitnot_number(i) {
    var x = ((i | 0) * (i >> 16)) | 0; // integer multiplication
    if (uceFault(i) || uceFault(i)) {
        x = (i | 0) * (i >> 16); // double multiplication during bailout
        print(x);
    }
    return x;
}
Refactoring patch needed for the next patch.
Attachment #8494701 - Flags: review?(sunfish)
When the truncation is not possible because we have removed uses, then we
check if we can recover the instruction, and clone it for the bailout path.
Attachment #8494704 - Flags: review?(sunfish)
Comment on attachment 8494704 [details] [diff] [review]
Recover truncated instruction.

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

I'm still working on understanding what this patch is doing. Here are a few preliminary comments.

::: js/src/jit/RangeAnalysis.cpp
@@ +2535,5 @@
> +            i++;
> +            continue;
> +        }
> +
> +        MUse *use = *i++;

Move this line to the top of the loop, and then replace the (*i)->consumer() with use->consumer(). Then you can remove the other i++, and it'll be easier to see how this loop "works".

@@ +2582,5 @@
> +    // that needs no conversion, we don't have to worry about resume points
> +    // seeing truncated values.
> +    bool needsConversion = !candidate->range() || !candidate->range()->isInt32();
> +
> +    // If the cnadidate instruction appears as operand of a resume point or a

Typo: candidate

@@ +2586,5 @@
> +    // If the cnadidate instruction appears as operand of a resume point or a
> +    // recover instruction, and we have to truncate its results.
> +    //
> +    // Then we might have to either recover it result during the bailout, or
> +    // avoid the truncation.

Stylo: Don't separate the "If" and its "Then" by a full-stop and blank line.
(In reply to Dan Gohman [:sunfish] from comment #3)
> I'm still working on understanding what this patch is doing. Here are a few
> preliminary comments.

This patch is looking at truncated instruction which have removed uses.  If needed, it checks if a copy of the instruction can be kept for the bailout path, such as, if there is a bailout, we recover the untruncated result of the operation and not the mistakenly truncated results, as seen in Bug 1007213.

This implies that if we remove unlikely/unreachable branches which are (potentially) using the double result, we can still improve our truncation results in the generated code.
Review ping.
This will potentially fix the octane-splay regression on x86, like nbp mentioned in bug 1072911 comment 7. Also we have a very big android regression. Hopefully this also fix that. So it might be good to get this in rather quick. In case it doesn't fix to have more time to investigate.
Comment on attachment 8494701 [details] [diff] [review]
IonMonkey: Split truncate function.

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

One thing that would help me a lot here is a clear description of the interface with needTruncation and truncate:

 - What state is needTruncation allowed to mutate?
 - What does the return value of needTruncation mean?
 - When is truncate called?

These are the things that are most important for someone adding or modifying a new MIR node kind, for example, so it'd be good to have comments for these.
Attachment #8494701 - Flags: review?(sunfish) → review+
Comment on attachment 8494704 [details] [diff] [review]
Recover truncated instruction.

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

Ok, I think this is ok, with earlier review comments addressed.
Attachment #8494704 - Flags: review?(sunfish) → review+
The previous 2 patches worked well already with Bug 1007213.  This patch is
the last missing piece to make it work correctly on comment 0.  Note that
without this patch the test case asserts, because baseline does not have
GVN, and the bailout compare the truncated result with the non-truncated
result.
Attachment #8498849 - Flags: review?(sunfish)
Attachment #8498849 - Flags: review?(sunfish) → review+
https://hg.mozilla.org/mozilla-central/rev/79097ed14588
https://hg.mozilla.org/mozilla-central/rev/e25f541006dc
https://hg.mozilla.org/mozilla-central/rev/54a2ae5e9c18
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Depends on: 1215921
You need to log in before you can comment on or make changes to this bug.