882565 - IonMonkey: Incorrect (0xffffffff > zero()) * (0xffffffff >>> x)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

function zero() { return 0; }
function f(x) { return (0xffffffff > zero()) * (0xffffffff >>> x); }
assertEq(f(0), f(0));

With --ion-eager: got 4294967295, expected 0


Found by the asm differential testing in jsfunfuzz, which I ran with --ion-eager just for kicks.  OdinMonkey got it right!


The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/cb36ad241f80
user:        Brian Hackett
date:        Mon Apr 15 05:37:53 2013 -0600
summary:     Merge m-i to ionmonkey

The bug was introduced by a merge (it was not present on either parent).
I don't know which patches from each side of the merge contributed to the bug. Sorry.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Patch

AllUsesTruncate incorrectly ignores resumepoint uses. In this case we have:

return (0xffffffff > zero()) * (0xffffffff >>> x);

The > operation is folded by GVN. AllUsesTruncate then returns true for the first 0xffffffff constant (a double), because it's only used by a resumepoint. Then we bailout before the JSOP_GT and use the truncated value.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Patch

The regression range is fault. It is caused by bug 841666, when truncate analysis was moved to range analysis. We assume resumepoints can be skipped, because they aren't observable, but they are when done in range analysis. UCE and GVN have happened before and could remove potential reasons to disable trunacating a definition.

- Patch from Jan fixes the correctness in all cases, but also introduces an enormous speed regression on v8-crypto

There are two possible fixes that don't regress crypto. 
1) Take fix from Jan for correctness and let the current truncate analysis look to resume points. To combat the speed regression we could reintroduce the old truncate analysis pass that happened before UCE/GVN.

2) Mark functions were UCE/GVN have removed an MUse from. When deciding if we can do truncate analysis without looking to ResumePoints, we need to be sure all uses are still present in the SSA. If the function is marked we cannot ignore ResumePoints.

Solution (1) would reintroduce a lot of code that will get removed again, since it is not the right fix. I think it could reintroduce a lot of new issues again. The old truncate analysis pass well is old and has its baggage. 

Solution (2) would be the right fix. Though to do this really good would also require a lot of code changes. Also something we don't want when backporting to aurora and beta. But I created a fix that is good enough for now. It fixes the correctness issue and the speed regression of crypto.

@Jan: I know Folded and UseRemoved look the same, but for this patch I want to keep them separated. Time is short to uplift it and I want to keep the difference and potential fall out as low as possible. Also marking the operands in replaceAllUsesWith as "UseRemoved" is actually a bit eagerly. It should normally only happen when block->discard() is called. But seemed easiest for now. Since doing it at block->discard() will potentially mark much much more unnecessary definitions. (discard is used in strange ways e.g. for phi's).
I'm targeting FF25 to improve this. To better distinguish discards that remove real Uses and discards that temporary remove it.

Comment 3

[Hannes Verschore [:h4writer]]

Attachment: Patch with testcases

I forgot to qref, so the testcases weren't added.

Comment 4

[Jan de Mooij [:jandem]]

Comment on attachment 762298 [details] [diff] [review]
Patch with testcases

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

Thanks for stealing this and fixing it in a way that does not regress perf!

::: js/src/ion/RangeAnalysis.cpp
@@ +1409,5 @@
>  {
> +    for (MUseIterator use(candidate->usesBegin()); use != candidate->usesEnd(); use++) {
> +        if (!use->consumer()->isDefinition()) {
> +            // We can only skip testing resume points, if all original uses are still present.
> +            // Only than testing all uses is enough to guarantee the truncation isn't observerable.

Nit: s/than/then and comments should fit within 80 columns.

::: js/src/jit-test/tests/ion/bug882565.js
@@ +1,3 @@
> +function zero() { return 0; }
> +function f(x) { return (0xffffffff > zero()) * (0xffffffff >>> x); }
> +assertEq(f(0), f(0));

Nit: rewrite this as

assertEq(f(0), ...);
assertEq(f(0), ...);

So that the test will fail if both calls return the same wrong value.

Comment 5

[Hannes Verschore [:h4writer]]

I'm currently not home, but this is a heads-up, since I heard the last beta will ship Monday? This bug is present in FF22, so we should try to fix it. I'm already setting flags to track this for FF22. And I will land and ask for approval for beta/aurora tomorrow. The patch should apply to both and risk is low.

Comment 6

[Hannes Verschore [:h4writer]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e2d22c56cc

Comment 7

[Hannes Verschore [:h4writer]]

Comment on attachment 762298 [details] [diff] [review]
Patch with testcases

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 841666

User impact if declined: Possible incorrect calculations in JS.

Testing completed (on m-c, etc.): m-i landed 2 hours ago. jit-tests ok. And awfy don't show a regression for any of our major benchmarks.


Risk to taking this patch (and alternatives if risky):
Change is as small as possible and disable some optimizations. No new behaviour, only disabling some optimizations in certain circumstances.
Risk would be limited to maybe performance regressions, but I think I cover almost all cases. And it is confirmed it doesn't hit our major benchmarks that we care about.

String or IDL/UUID changes made by this patch: /

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d8e2d22c56cc

Comment 9

[Alex Keybl [:akeybl]]

Comment on attachment 762298 [details] [diff] [review]
Patch with testcases

We don't know of any impacted user flows or major web regressions, so we'll only take this on FF23, not FF22. Sad that we take this regression, but we want to be as safe as possible in our final beta.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17adfc82ed91