Last Comment Bug 736420 - IonMonkey: Make multiplies more efficient on ARM
: IonMonkey: Make multiplies more efficient on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-16 03:28 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-27 18:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/MoreMul-r0.patch (4.40 KB, patch)
2012-03-16 03:28 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-03-16 03:28:10 PDT
Created attachment 606516 [details] [diff] [review]
/home/mrosenberg/patches/MoreMul-r0.patch

Make multiplies by a constant more efficient on ARM by changing (x * (1<<y)) into
lsl dest, x, #y; cmp  x (dst asr #y) and the known non-overflow case of (x* (1<<y | 1 << z)) into add dest, x, x lsl #(y-z); lsl dest, dest, #z. I can't think of any surefire way of detecting overflow in this case, but I may be able to get away with seeing if the next higher power of two overflows. I ran this optimization on sunspider and v8, but I didn't see a noticeable improvement, so for now this patch is at best a "make me feel better about the actual instructions that we generate" type of patch.
Comment 1 Jacob Bramley [:jbramley] 2012-03-16 07:40:48 PDT
Comment on attachment 606516 [details] [diff] [review]
/home/mrosenberg/patches/MoreMul-r0.patch

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

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +353,5 @@
>      const LAllocation *lhs = ins->getOperand(0);
>      const LAllocation *rhs = ins->getOperand(1);
>      const LDefinition *dest = ins->getDef(0);
>      MMul *mul = ins->mir();
> +    bool handled = false;

This ought to be in a scope local to where it's used (such as after the 'default' case). For the cases that don't use it, 'handled' can still be false even though the code has been handled, and this might be confusing later.

@@ +392,5 @@
> +                    handled = true;
> +                } else {
> +                    uint32 shift_rest;
> +                    JS_FLOOR_LOG2(shift_rest, rest);
> +                    if ((1 << shift_rest) == rest) {

Perhaps add a comment stating that this handles the case where the operand is a combination of two powers of two.

@@ +401,5 @@
> +                    }
> +                }
> +            } else if (ToRegister(lhs) != ToRegister(dest)) {
> +                // To stay on the safe side, only optimize things that are a
> +                // power of 2.

A more common case (I suspect) is multiplication by small numbers which aren't necessarily powers of two. These are quite easy to implement using ADD and Operand 2. Still, it isn't likely to bump the benchmarks much.

@@ +408,3 @@
>                  JS_FLOOR_LOG2(shift, constant);
>                  if ((1 << shift) == constant) {
> +                    // dest = lhs * (2**shift)

The '**' operator doesn't exist in C, so it's probably best not to use it in comments.

@@ +411,2 @@
>                      masm.ma_lsl(Imm32(shift), ToRegister(lhs), ToRegister(dest));
> +                    // assert (lhs == dest >> shift)

I don't see an assertion.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-07-27 18:03:20 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/bc22b7e24bc3
Comment 3 Marty Rosenberg [:mjrosenb] 2012-07-27 18:55:46 PDT
whoops (again)

Note You need to log in before you can comment on or make changes to this bug.