1294042 - Implement the folding of MPow

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Hannes Verschore [:h4writer]]

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".

Comment 1

[Taahir Ahmed]

Attachment: 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?

Comment 2

[Hannes Verschore [:h4writer]]

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?

Comment 3

[Hannes Verschore [:h4writer]]

(sorry it took me that long to review)

Comment 4

[Hannes Verschore [:h4writer]]

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!

Comment 5

[Taahir Ahmed]

No blockers --- it just slipped my mind.  I will try to fix up the patch this weekend.

Comment 6

[Hannes Verschore [:h4writer]]

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!

Comment 7

[Taahir Ahmed]

Attachment: 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()`.

Comment 8

[Hannes Verschore [:h4writer]]

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;

Comment 9

[Taahir Ahmed]

Attachment: pow-fold-2016-11-10.patch

I have applied your feedback

Comment 10

[Hannes Verschore [:h4writer]]

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 "!"

Comment 11

[Hannes Verschore [:h4writer]]

Properly assigning this bug to Taahir.
@Taahir: thank you for the help!

Comment 12

[Taahir Ahmed]

Attachment: 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.

Comment 13

[Hannes Verschore [:h4writer]]

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.

Comment 14

[Hannes Verschore [:h4writer]]

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".

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/63a584c9d1f8
Implement constant folding for MPow r=h4writer

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/63a584c9d1f8

Comment 17

[areya]

https://hg.mozilla.org/mozilla-central/rev/63a584c9d1f8