Bug 101871 – DFG ArithMul overflow check elimination is too aggressive

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

It appears to ignore the fact that (a * b) | 0 is not always the same as ((a | 0) * (b | 0)) | 0.  For all of the places where this matters, it's easy to keep the optimization, just by making the compiler a bit smarter about when and how to use it.

[Filip Pizlo]

Created attachment 173523 [details]
the patch

Can I also get a rubber stamp for the set of tests I'm hacking up for this?

[Filip Pizlo]

Comment on attachment 173523 [details]
the patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173523&action=review

> Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp:147
> +        case JSConstant: {
> +            JSValue immediateValue = node.valueOfJSConstant(codeBlock());
> +            if (!immediateValue.isInt32())
> +                return false;
> +            int32_t intImmediate = immediateValue.asInt32();
> +            return intImmediate > -(1 << power) && intImmediate < (1 << power);
> +        }

I meant to have this case call isWithinPowerOfTwoForConstant().

[Filip Pizlo]

Created attachment 173524 [details]
the patch

Again, asking for a rubber stamp for tests.  I still haven't layouttest-ified them.

[WebKit Review Bot]

Comment on attachment 173524 [details]
the patch

Attachment 173524 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14803518

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Filip Pizlo]

Created attachment 173550 [details]
the patch

Now, including tests!

[Filip Pizlo]

Landed in http://trac.webkit.org/changeset/134314