Last Comment Bug 698579 - IonMonkey: implement the mul family of instructions on ARM
: IonMonkey: implement the mul family of instructions on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All Linux
: -- normal (vote)
: ---
Assigned To: general
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-31 13:54 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2011-11-10 15:38 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implement multiply and split shifts (41.22 KB, patch)
2011-10-31 13:54 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Review

Description Marty Rosenberg [:mjrosenb] 2011-10-31 13:54:39 PDT
Created attachment 570836 [details] [diff] [review]
implement multiply and split shifts

I was saving this for last, since it should be relatively easy; I have yet to implement multiply. There are likely going to be two distinct paths for multiply; the overflow and non-overflow cases.  In the non-overflow case, a simple mul is sufficient.  For cases where the multiply may overflow, We need to generate a 64 bit result, and manually verify that
the 64 bit result is in fact a sign extended 32 bit result.

This patch also splits a shift into an and followed by a shift, since on arm, x << y is implicitly x << (y & 255) rather than x << (y & 31) as on x86.
Comment 1 Jacob Bramley [:jbramley] 2011-11-01 03:26:33 PDT
Comment on attachment 570836 [details] [diff] [review]
implement multiply and split shifts

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

The HG comment doesn't mention MUL.

There are a few bits which look like they belong in other patches. They're really small and they can go in this one, but they might belong in other MQ patches.

There are one or two errors in the patch but it's generally good. I'm giving r+ assuming that those errors are fixed before it's pushed. (They're all trivial.)

::: js/src/ion/Lowering.h
@@ +6,5 @@
>   *
>   * The contents of this file are subject to the Mozilla Public License Version
>   * 1.1 (the "License"); you may not use this file except in compliance with
>   * the License. You may obtain a copy of the License at
> + * http://www.mozilla.org/MPL/b

This looks like a mistake.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +833,5 @@
> +Assembler::as_genmul(Register dhi, Register dlo, Register rm, Register rn,
> +          MULOp op, SetCond_ sc, Condition c)
> +{
> +
> +    writeInst(RN(dhi) | maybeRD(dlo) | RM(rm) | rn.code() | op | sc | c);

I cannot find a reference to maybeRD. Was it added in a previous patch that I haven't yet seen?

@@ +850,5 @@
> +}
> +void
> +Assembler::as_umaal(Register destHI, Register destLO, Register src1, Register src2,
> +                SetCond_ sc, Condition c)
> +{

Add ASSERT(!sc) or just remove it. (UMAAL cannot set flags.)

@@ +856,5 @@
> +}
> +void
> +Assembler::as_mls(Register dest, Register acc, Register src1, Register src2,
> +                SetCond_ sc, Condition c)
> +{

Add ASSERT(!sc) or just remove it. (MLS cannot set flags.)

@@ +857,5 @@
> +void
> +Assembler::as_mls(Register dest, Register acc, Register src1, Register src2,
> +                SetCond_ sc, Condition c)
> +{
> +    as_genmul(dest, acc, src1, src2, opm_mla, sc, c);

opm_mls, surely.

::: js/src/ion/arm/Assembler-arm.h
@@ +313,5 @@
> +    opm_umull = 4 << 21,
> +    opm_umlal = 5 << 21,
> +    opm_smull = 6 << 21,
> +    opm_smlal = 7 << 21
> +};

All of these also have bits 4-7 set to 0b1001, but I don't think you add that in the writeInst part. It probably belongs here.

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +426,5 @@
>                      masm.ma_lsl(Imm32(shift), ToRegister(lhs), ToRegister(dest));
>                      return true;
>                  }
> +            }
> +            else if (!mul->canOverflow()) {

This change is just bracket style, and the original was better, I think.

@@ +560,5 @@
>        case JSOP_BITAND:
>          if (rhs->isConstant()) {
> +            //            if (rhs->isConstantIndex()) {
> +            //                masm.ma_and(Imm32(31), ToRegister(lhs), ToRegister(dest));
> +            //            } else {

Did you mean to leave this commented code behind?

::: js/src/ion/arm/Lowering-arm.cpp
@@ +274,4 @@
>          ins->setOperand(1, useOrConstant(rhs));
> +    } else {
> +        // If our operand isn't a constant, then we want to strip off any
> +        // unnecessary bits.  We do this by generating an AND LIrstruction

s/LIrstruction/LInstruction/

@@ +281,5 @@
> +        LAnd->setOperand(1, LConstantIndex::FromIndex(0x1f));
> +        LAnd->setDef(0, LDefinition(LDefinition::TypeFrom(mir->type()), LDefinition::DEFAULT));
> +        uint32 vreg = getVirtualRegister();
> +        if (vreg >= MAX_VIRTUAL_REGISTERS)
> +            return false;

Curly brackets.

@@ +284,5 @@
> +        if (vreg >= MAX_VIRTUAL_REGISTERS)
> +            return false;
> +        LAnd->getDef(0)->setVirtualRegister(vreg);
> +        if (!add(LAnd))
> +            return false;

Curly brackets.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +475,5 @@
>  }
>  void
>  MacroAssemblerARM::ma_rsb(Register src1, Register src2, Register dest, SetCond_ sc, Condition c)
>  {
> +    as_alu(dest, src1, O2Reg(src2), op_rsc, sc, c);

Also: s/op_rsc/op_rsb/

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