822436 - IonMonkey: Inline Math.imul

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

We should follow through with our plan in Bug 808148 and make Math.imul super awesome.

Comment 1

[Luke Wagner [:luke]]

Random question: do you know if there is a V8 bug on file to implement Math.imul?  Because it'd be totally super awesome if they had it too :)

Comment 2

[Tom S [:evilpie]]

Attachment: v1

So we discussed this a while back in #ionmonkey. I settled on reusing MMul with a new "Integer" mode.

Comment 3

[Hannes Verschore [:h4writer]]

Sorry, I know you didn't ask a review, but I happened to pass by and look to the code and had a small comment ;). I really like the fact we can reuse MMul for this :D.

I would prefer to set canBeNegativeZero_ to false, possibleTruncates_ to true and impliciteTruncates_ to true in the constructor of MMul when mode is set to "Integer". (That also removes the "if's" in fallible/canBeNegativeZero/truncates ...) 
In that case make sure that "implicitTruncates" isn't overwritten by "possibleTruncates" being true in the truncate pass.

Comment 4

[Tom S [:evilpie]]

Attachment: v2

I think i finally convinced myself that this is the way to go. And you just qualified to review this :)

Comment 5

[Hannes Verschore [:h4writer]]

Comment on attachment 697471 [details] [diff] [review]
v2

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

Looks good :D.

::: js/src/ion/MCallOptimize.cpp
@@ +663,5 @@
> +    MInstruction *first = MTruncateToInt32::New(argv[1]);
> +    current->add(first);
> +
> +    MInstruction *second = MTruncateToInt32::New(argv[2]);
> +    current->add(second);

s/first/lhs/, s/second/rhs/

@@ +687,2 @@
>          return InliningStatus_NotInlined;
> +    MIRType arg2Type = getInlineArgType(argc, 1);

MIRType arg2Type = getInlineArgType(argc, 2);

::: js/src/ion/MIR.h
@@ +2320,5 @@
>  
>      TypePolicy *typePolicy() {
>          return this;
>      }
> +    bool congruent(MDefinition *const &ins) const {

This seems incorrect and should be congruentTo

@@ +2640,5 @@
> +        } else {
> +            JS_ASSERT(mode == Integer);
> +            canBeNegativeZero_ = false;
> +            possibleTruncate_ = implicitTruncate_ = true;
> +        }

Please keep setting the default values and only change them if (mode == Integer) to the specialized version. A comment about what Integer mode is and why we can put these variables would be good too ;)

Comment 6

[Tom S [:evilpie]]

Comment on attachment 697471 [details] [diff] [review]
v2

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

Thanks for the quick review.

::: js/src/ion/MCallOptimize.cpp
@@ +663,5 @@
> +    MInstruction *first = MTruncateToInt32::New(argv[1]);
> +    current->add(first);
> +
> +    MInstruction *second = MTruncateToInt32::New(argv[2]);
> +    current->add(second);

This is not a binary instruction, but a function parameter. There is no lefthandside and righthandside.

@@ +687,2 @@
>          return InliningStatus_NotInlined;
> +    MIRType arg2Type = getInlineArgType(argc, 1);

D'uh stupid, thank you for catching this!

::: js/src/ion/MIR.h
@@ +2640,5 @@
> +        } else {
> +            JS_ASSERT(mode == Integer);
> +            canBeNegativeZero_ = false;
> +            possibleTruncate_ = implicitTruncate_ = true;
> +        }

Done

Comment 7

[Tom S [:evilpie]]

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

Comment 8

[Alon Zakai (:azakai)]

(In reply to Luke Wagner [:luke] from comment #1)
> Random question: do you know if there is a V8 bug on file to implement
> Math.imul?  Because it'd be totally super awesome if they had it too :)

A saw that evilpie filed a bug at:

http://code.google.com/p/v8/issues/detail?id=2455

(thanks!)

Comment 9

[Ed Morley [:emorley]]

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