Closed Bug 1294042 Opened 7 years ago Closed 7 years ago

Implement the folding of MPow


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




Tracking Status
firefox53 --- fixed


(Reporter: h4writer, Assigned: ahmed.taahir, Mentored)


(Blocks 1 open bug)


(Keywords: perf)


(1 file, 3 obsolete files)

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".
Mentor: hv1989
Attached patch pow-fold-2016-08-21.patch (obsolete) — Splinter Review
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)
Comment on attachment 8783273 [details] [diff] [review]

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


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+
(sorry it took me that long to review)
Blocks: jsperf
Keywords: perf
Priority: -- → P5
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)
No blockers --- it just slipped my mind.  I will try to fix up the patch this weekend.
I've created a wiki page about how to implement this. Might be helpful as a start.

Thanks again for volunteering and I hope you find some time to dive into this. Good luck!
Attached patch pow-fold-2016-11-09.patch (obsolete) — Splinter Review
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)
Comment on attachment 8809303 [details] [diff] [review]

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:
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)
Attached patch pow-fold-2016-11-10.patch (obsolete) — Splinter Review
I have applied your feedback
Attachment #8809303 - Attachment is obsolete: true
Attachment #8809483 - Flags: review?(hv1989)
Comment on attachment 8809483 [details] [diff] [review]

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+
Properly assigning this bug to Taahir.
@Taahir: thank you for the help!
Assignee: nobody → ahmed.taahir
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)
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: That will allow you to push to try server. I have done that now for you:
- 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.
Attachment #8810466 - Flags: checkin?(hv1989)
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
Pushed by
Implement constant folding for MPow r=h4writer
Keywords: checkin-needed
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.