Implement the folding of MPow

RESOLVED FIXED in Firefox 53

Status

()

Core
JavaScript Engine: JIT
P5
normal
RESOLVED FIXED
a year ago
9 months ago

People

(Reporter: h4writer, Assigned: Taahir Ahmed, Mentored)

Tracking

(Blocks: 1 bug, {perf})

unspecified
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

a year ago
In IonMonkey we can optimize instructions based on their inputs. We often do this for constant. Like "MSub 8, 9" will be pre-computed to "MConstant -1". We do this already for a lot of instructions, but apparently not for MPow.

This code for this is in js/src/jit/MIR.cpp and the function doing this is "foldsTo".
(Reporter)

Updated

a year ago
Mentor: hv1989@gmail.com
(Assignee)

Comment 1

a year ago
Created attachment 8783273 [details] [diff] [review]
pow-fold-2016-08-21.patch

Here's an initial stab at it.  The output in the all-constant case is always a double constant --- Should the output type vary based on the result type of the MPow instruction?
Attachment #8783273 - Flags: review?(hv1989)
(Reporter)

Comment 2

a year ago
Comment on attachment 8783273 [details] [diff] [review]
pow-fold-2016-08-21.patch

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

Nice!

Some suggestions:
Instead of using static functions, can you use methods of MPow, like: tryFoldConstant and tryFoldConstantPower.
Both returning "nullptr" when replacing failed.
In that case you can make foldsTo just calling these functions and checking for nullptr and on nullptr doing the next lesser optimized fold?
Attachment #8783273 - Flags: review?(hv1989) → feedback+
(Reporter)

Comment 3

a year ago
(sorry it took me that long to review)

Updated

11 months ago
Blocks: 1307062
Keywords: perf
Priority: -- → P5
(Reporter)

Comment 4

10 months ago
Could it be this slipped from you radar? The patch you have created is quite far already. It would be a shame not getting this in the tree. Is something blocking you from finishing this? Thanks!
Flags: needinfo?(ahmed.taahir)
(Assignee)

Comment 5

10 months ago
No blockers --- it just slipped my mind.  I will try to fix up the patch this weekend.
(Reporter)

Comment 6

10 months ago
I've created a wiki page about how to implement this. Might be helpful as a start.
https://wiki.mozilla.org/IonMonkey/Global_value_numbering

Thanks again for volunteering and I hope you find some time to dive into this. Good luck!
(Assignee)

Comment 7

9 months ago
Created attachment 8809303 [details] [diff] [review]
pow-fold-2016-11-09.patch

Feedback applied.  I'm not sure that pushing the conditionals into the helper functions made the patch simpler --- there are still just as many branches in the main `foldsTo()`.
Attachment #8783273 - Attachment is obsolete: true
Flags: needinfo?(ahmed.taahir)
Attachment #8809303 - Flags: review?(hv1989)
(Reporter)

Comment 8

9 months ago
Comment on attachment 8809303 [details] [diff] [review]
pow-fold-2016-11-09.patch

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

Thanks a lot. Looking better already!

The previous feedback and this feedback is more that of consistency in the coding style.
Having the same style gives some familiarity and makes it easier to read and understand the code.
The logic in the previous and this patch is sound IMO.

The coding style we use is visible at:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style
In most cases that agrees to the internal coding style we use in the JS engine.
If I see code that happens to following the coding style but is used in the JS engine, I'll will give a small style nit.

Seems like you added "this->" everywhere. That is actually not needed.
Can you remove all unneeded "this->" in the code?

::: js/src/jit/MIR.cpp
@@ +3200,5 @@
>  {
> +    bool pConst = this->power()->isConstant()
> +        && this->power()->toConstant()->isTypeRepresentableAsDouble();
> +    bool xConst = this->input()->isConstant()
> +        && this->input()->toConstant()->isTypeRepresentableAsDouble();

instead of creating a variable, can you test and "return this" immediately?

if (!power()->isConstant() || !input()->isConstant())
    return this;
if (!power()->toConstant()->isTypeRepresentableAsDouble())
    return nullptr;
if (!input()->toConstant()->isTypeRepresentableAsDouble())
    return nullptr;

@@ +3205,5 @@
>  
> +    // Both `x` and `p` in `x^p` must be constants in order to precompute.
> +    if(!(pConst && xConst)) {
> +        return nullptr;
> +    }

This can now go. But if you saw in my previous comment. If a conditional is only followed by one line, we omit the brackets.

@@ +3221,5 @@
> +
> +    // If `p` in `x^p` isn't constant, we can't apply these folds.
> +    if(!pConst) {
> +        return nullptr;
> +    }

Please put the condition in the if statement.
Feel free to split into two conditions to make it more readable.
Style nit: if the condition and the body each have one line, omit the brackets.

@@ +3223,5 @@
> +    if(!pConst) {
> +        return nullptr;
> +    }
> +
> +    double pow = this->power()->toConstant()->numberToDouble();

Nit: remove this->

@@ +3224,5 @@
> +        return nullptr;
> +    }
> +
> +    double pow = this->power()->toConstant()->numberToDouble();
> +    MIRType outputType = this->type();

Nit: remove this->

@@ +3229,4 @@
>  
>      // Math.pow(x, 0.5) is a sqrt with edge-case detection.
>      if (pow == 0.5)
> +        return MPowHalf::New(alloc, this->input());

Nit: remove this->

@@ +3238,2 @@
>          MConstant* one = MConstant::New(alloc, DoubleValue(1.0));
> +        this->block()->insertBefore(this, one);

No need to touch this code. It hasn't changed.

@@ +3254,5 @@
> +                               this->input(),
> +                               this->input(),
> +                               outputType);
> +        this->block()->insertBefore(this, mul1);
> +        return MMul::New(alloc, this->input(), mul1, outputType);

No need to touch this code. It hasn't changed.

@@ +3260,5 @@
>  
>      // Math.pow(x, 4) == y*y, where y = x*x.
>      if (pow == 4.0) {
> +        MMul* y = MMul::New(alloc, this->input(), this->input(), outputType);
> +        this->block()->insertBefore(this, y);

No need to touch this code. It hasn't changed.

@@ +3273,5 @@
> +MPow::foldsTo(TempAllocator& alloc)
> +{
> +    return this->tryFoldConstXConstP(alloc)
> +        ? this->tryFoldConstP(alloc)
> +        : this;

Please try to match how similar functions are constructed.

if (MDefinition* def = foldsConstant())
    return def;
if (MDefinition* def = foldsConstantPower())
    return def;
return this;
Attachment #8809303 - Flags: review?(hv1989)
(Assignee)

Comment 9

9 months ago
Created attachment 8809483 [details] [diff] [review]
pow-fold-2016-11-10.patch

I have applied your feedback
Attachment #8809303 - Attachment is obsolete: true
Attachment #8809483 - Flags: review?(hv1989)
(Reporter)

Comment 10

9 months ago
Comment on attachment 8809483 [details] [diff] [review]
pow-fold-2016-11-10.patch

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

Can you reupload with the minor change for every "!" and add r=h4writer to the end of the commit message?

::: js/src/jit/MIR.cpp
@@ +3200,3 @@
>  {
> +    // Both `x` and `p` in `x^p` must be constants in order to precompute.
> +    if(! (input()->isConstant() && power()->isConstant()))

Nit here and below. We don't add a space after "!"
Attachment #8809483 - Flags: review?(hv1989) → review+
(Reporter)

Comment 11

9 months ago
Properly assigning this bug to Taahir.
@Taahir: thank you for the help!
Assignee: nobody → ahmed.taahir
(Assignee)

Comment 12

9 months ago
Created attachment 8810466 [details] [diff] [review]
0001-Bug-1294042-Implement-constant-folding-for-MPow-r-h4.patch

Thanks for helping me get this into shape.  I'm sorry if setting the checking flag was the wrong thing to do --- I am making this up as I go along.
Attachment #8809483 - Attachment is obsolete: true
Attachment #8810466 - Flags: checkin?(hv1989)
(Reporter)

Comment 13

9 months ago
Sure. I know it can be hard to find what the procedure is. You'll learn bit by bit by doing the wrong and right things. Sorry to not make it easier for you.

I'll give you a quick summary of what will happen:
- You can run jit-tests locally to see if you'll hit problems.
- Afterwards you'll have to push to try server, which will tests for issues on different platforms. You can request level 1 commit access quite quickly: https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/. That will allow you to push to try server. I have done that now for you: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f6e4e227ea7f90ab1f01247272a9e0f035a2d4c
- If that is green/passes you can request to get it into the tree. That is possible if you have commit access level 3. Which will take a while before somebody will vouch for you. In the meantime you can put the keyword "checkin-needed" in the keyword list of the bug report. A sheriff will push your patch in that case.

Now you'll have to wait for the results to roll in.
(Reporter)

Updated

9 months ago
Attachment #8810466 - Flags: checkin?(hv1989)
(Reporter)

Comment 14

9 months ago
The try run looks green. Added the checkin-needed keyword. It should get into mozilla-inbound in a few hours / a day. After that it will ride the trains until it is released in FF53.

In the meantime you might be interested in doing a similar bug report? Bug 1277086 is about folding "0 - x" to "x * -1".
Keywords: checkin-needed

Comment 15

9 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a584c9d1f8
Implement constant folding for MPow r=h4writer
Keywords: checkin-needed

Comment 16

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/63a584c9d1f8
Status: NEW → RESOLVED
Last Resolved: 9 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 17

9 months ago
https://hg.mozilla.org/mozilla-central/rev/63a584c9d1f8
You need to log in before you can comment on or make changes to this bug.