Closed Bug 822436 Opened 7 years ago Closed 7 years ago

IonMonkey: Inline Math.imul

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(1 file, 1 obsolete file)

We should follow through with our plan in Bug 808148 and make Math.imul super awesome.
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 :)
Depends on: 808148
Attached patch v1 (obsolete) — Splinter Review
So we discussed this a while back in #ionmonkey. I settled on reusing MMul with a new "Integer" mode.
Assignee: general → evilpies
Status: NEW → ASSIGNED
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.
Attached patch v2Splinter Review
I think i finally convinced myself that this is the way to go. And you just qualified to review this :)
Attachment #695636 - Attachment is obsolete: true
Attachment #697471 - Flags: review?(hv1989)
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 ;)
Attachment #697471 - Flags: review?(hv1989) → review+
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
(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!)
https://hg.mozilla.org/mozilla-central/rev/b9c4a9483492
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Depends on: 886243
Depends on: 940642
You need to log in before you can comment on or make changes to this bug.