Closed Bug 736420 Opened 12 years ago Closed 12 years ago

IonMonkey: Make multiplies more efficient on ARM

Categories

(Core :: JavaScript Engine, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mjrosenb, Unassigned)

Details

Attachments

(1 file)

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.
Attachment #606516 - Flags: review?(Jacob.Bramley)
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.
Attachment #606516 - Flags: review?(Jacob.Bramley) → review+
whoops (again)
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.