Last Comment Bug 714949 - IonMonkey: Implement {Load,Store}element{T,V} on ARM
: IonMonkey: Implement {Load,Store}element{T,V} on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM Linux
: -- normal (vote)
: ---
Assigned To: general
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-03 13:53 PST by Marty Rosenberg [:mjrosenb]
Modified: 2012-01-08 18:17 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implement the function LoadStoreElement functions (32.90 KB, patch)
2012-01-03 13:53 PST, Marty Rosenberg [:mjrosenb]
sstangl: review+
Details | Diff | Splinter Review

Description Marty Rosenberg [:mjrosenb] 2012-01-03 13:53:06 PST
Created attachment 585546 [details] [diff] [review]
implement the function LoadStoreElement functions

I've also bundled up a few minor fixes that I noticed while testing.
Comment 1 Sean Stangl [:sstangl] 2012-01-05 23:00:01 PST
Comment on attachment 585546 [details] [diff] [review]
implement the function LoadStoreElement functions

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

This patch seems OK. I would prefer that archUseOrConstant() not exist, since it doesn't map nicely to a sensible abstraction and is extremely specific in the first place.

I obviously don't know ARM well, so I looked at those sections only on the surface, assuming that operations don't have any tricky semantics. Most feedback is about commenting style and whitespace.

::: js/src/ion/CodeGenerator.cpp
@@ +488,5 @@
>          static const VMFunction InvokeFunctionInfo = FunctionInfo<pf>(InvokeFunction);
>  
>          // Nestle %esp up to the argument vector.
>          // Each path must account for framePushed_ separately, for callVM to be valid.
> +

No added space here: the above comments are for the call to freeStack().

::: js/src/ion/IonMacroAssembler.cpp
@@ +113,5 @@
>              diff += sizeof(double);
>          else
>              diff += STACK_SLOT_SIZE;
>      }
> +    size_t new_diff = (diff + 7) & ~7;

This deserves a comment.

::: js/src/ion/Lowering.cpp
@@ +208,5 @@
>          MDefinition *right = comp->getOperand(1);
>  
>          if (comp->specialization() == MIRType_Int32) {
>              JSOp op = ReorderComparison(comp->jsop(), &left, &right);
> +            return add(new LCompareIAndBranch(op, useRegister(left), archUseOrConstant(right),

I don't like this: "archUseOrConstant()" isn't a meaningful directive. The use case seems too contrived for a meaningful abstration.

An #ifdef JS_CPU_ARM here would be clearer.

@@ +251,5 @@
>              return emitAtUses(comp);
>  
>          if (comp->specialization() == MIRType_Int32) {
>              JSOp op = ReorderComparison(comp->jsop(), &left, &right);
> +            return define(new LCompareI(op, useRegister(left), archUseOrConstant(right)), comp);

Same comment as above.

@@ +453,5 @@
>  bool
>  LIRGenerator::visitMod(MMod *ins)
>  {
>      MDefinition *lhs = ins->lhs();
> +    DebugOnly<MDefinition*> rhs = ins->rhs();

This can just be removed; rhs is unused even in debug builds. lhs should be DebugOnly. Additionally the open curly brace should be on a newline lining up with "if", while we're at it.

::: js/src/ion/arm/Assembler-arm.h
@@ +1006,5 @@
>          *dest = Imm32(offset);
>      }
> +    Address toAddress() const {
> +        return Address(Register::FromCode(reg), offset);
> +    }

It shouldn't be necessary to construct an Address from an Operand. They are not used interchangeably.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +551,5 @@
> +        ma_ldr(op, ScratchRegister);
> +        as_cmp(src1, O2Reg(ScratchRegister), c);
> +        break;
> +      default:
> +        JS_NOT_REACHED("trying to compare FP and integer registers");

Somewhat customary to leave a "break;" even in the JS_NOT_REACHED() case.

@@ +729,5 @@
>  {
>      ma_dtr(IsStore, rt, addr, mode, cc);
>  }
> +void
> +MacroAssemblerARM::ma_strd(Register rt, DebugOnly<Register> rt2, EDtrAddr addr, Index mode, Condition cc)

Having DebugOnly arguments is strange. If rt2 isn't explicitly used by as_extdtr, what effect does its value have?

@@ +1339,5 @@
>  void
>  MacroAssemblerARMCompat::loadInt32OrDouble(const Operand &src, const FloatRegister &dest)
>  {
>      Label notInt32, end;
> +    // TODO: try to rewrite this without branches.

Without TODO. (In general, we try to only leave TODO when there is a bug number associated with the comment. Occasionally some TODOs sneak in, but it's frowned upon.)

@@ +1360,5 @@
> +    Label notInt32, end;
> +    JS_STATIC_ASSERT(NUNBOX32_PAYLOAD_OFFSET == 0);
> +    // If it's an int, convert it to double.
> +    ma_alu(base, lsl(index, shift), ScratchRegister, op_add);
> +    // this is why only having one scratch register is sad.

Although accurate, not a useful comment :)

@@ +1366,5 @@
> +    branchTestInt32(Assembler::NotEqual, ScratchRegister, &notInt32);
> +    // this implicitly assumes that NUNBOX32_PAYLOAD_OFFSET == 0, since we don't give it an offset
> +    ma_ldr(DTRAddr(base, DtrRegImmShift(index, LSL, shift)), ScratchRegister);
> +    convertInt32ToDouble(ScratchRegister, dest);
> +    ma_b(&end);

This function could benefit from additional whitespace: newlines after "Label", JS_STATIC_ASSERT(), branchTestInt32. Comments should be formatted as if they're whole sentences, without contractions or "we/I", e.g., "Requires NUNBOX32_PAYLOAD_OFFSET == 0: no offset provided."

@@ +1448,5 @@
> +void
> +MacroAssemblerARMCompat::storeValue(ValueOperand val, Register base, Register index, int32 shift)
> +{
> +    
> +    if (((val.payloadReg().code() & 1) == 0) &&

It would be great if "1" had a name. What does it mean to have the LSB set?

@@ +1449,5 @@
> +MacroAssemblerARMCompat::storeValue(ValueOperand val, Register base, Register index, int32 shift)
> +{
> +    
> +    if (((val.payloadReg().code() & 1) == 0) &&
> +        val.typeReg().code() == val.payloadReg().code()+1)

nit: spacing around arithmetic.

@@ +1463,5 @@
> +            tmpIdx = ScratchRegister;
> +        }
> +        ma_strd(val.payloadReg(), val.typeReg(), EDtrAddr(base, EDtrOffReg(tmpIdx)));
> +    } else {
> +        // The ideal case is base is dead so we can do:

"is base is dead"

@@ +1470,5 @@
> +        // Sadly, we can't get this information, so we need to shuffle stuff around
> +        ma_alu(base, lsl(index, shift), ScratchRegister, op_add);
> +        // we know how to handle this case!
> +        storeValue(val, Address(ScratchRegister, 0));
> +        // TODO: see if we can use stm

Without TODO, unless bug filed.

@@ +1477,5 @@
> +
> +void
> +MacroAssemblerARMCompat::loadValue(Register base, Register index, ValueOperand val, int32 shift)
> +{
> +    if (((val.payloadReg().code() & 1) == 0) &&

:hardcoded_constants

@@ +1478,5 @@
> +void
> +MacroAssemblerARMCompat::loadValue(Register base, Register index, ValueOperand val, int32 shift)
> +{
> +    if (((val.payloadReg().code() & 1) == 0) &&
> +        val.typeReg().code() == val.payloadReg().code()+1)

nit: spacing around arithmetic.

@@ +1497,5 @@
> +        // ldr val.payloadReg(), [base, index LSL shift]!
> +        // ldr val.typeReg(), [base, #4]
> +        // Sadly, we can't get this information, so we need to shuffle stuff around
> +        ma_alu(base, lsl(index, shift), ScratchRegister, op_add);
> +        // we know how to handle this case!

Comments require fixing.

@@ +1499,5 @@
> +        // Sadly, we can't get this information, so we need to shuffle stuff around
> +        ma_alu(base, lsl(index, shift), ScratchRegister, op_add);
> +        // we know how to handle this case!
> +        loadValue(Address(ScratchRegister, 0), val);
> +        // TODO: see if we can use ldm

Without TODO, unless bug filed.

@@ +1592,5 @@
>  
>  }
> +
> +void
> +MacroAssemblerARMCompat::storePayload(const Value &val, Register base, Register index, int32 shift) {

nit: { on newline.

@@ +1599,5 @@
> +        ma_mov(ImmGCPtr((gc::Cell *)jv.s.payload.ptr), ScratchRegister);
> +    } else {
> +        ma_mov(Imm32(jv.s.payload.i32), ScratchRegister);
> +    }
> +    JS_STATIC_ASSERT(NUNBOX32_PAYLOAD_OFFSET == 0);

A comment explaining the utility of this assumption would be good.

@@ +1603,5 @@
> +    JS_STATIC_ASSERT(NUNBOX32_PAYLOAD_OFFSET == 0);
> +    as_dtr(IsStore, 32, Offset, ScratchRegister, DTRAddr(base, DtrRegImmShift(index, LSL, shift)));
> +}
> +void
> +MacroAssemblerARMCompat::storePayload(Register src, Register base, Register index, int32 shift) {

nit: { on newline.

@@ +1605,5 @@
> +}
> +void
> +MacroAssemblerARMCompat::storePayload(Register src, Register base, Register index, int32 shift) {
> +    JS_ASSERT(shift < 32);
> +    JS_ASSERT(shift >= 0);

nit: can combine above two assertions.

@@ +1607,5 @@
> +MacroAssemblerARMCompat::storePayload(Register src, Register base, Register index, int32 shift) {
> +    JS_ASSERT(shift < 32);
> +    JS_ASSERT(shift >= 0);
> +    JS_STATIC_ASSERT(NUNBOX32_PAYLOAD_OFFSET == 0);
> +    // technically, we can handle shift > -32 by changing LSL to ASR, but *effort*

32, not -32, right? This comment could use a bit of professionalization. Not to keep harping on the comments :)

@@ +1627,5 @@
>  void
> +MacroAssemblerARMCompat::storeTypeTag(ImmTag tag, Register base, Register index, int32 shift) {
> +    JS_ASSERT(base != ScratchRegister);
> +    JS_ASSERT(index != ScratchRegister);
> +    // we need to store a value int base + index << shift + 4.  HOW AWKWARD

We have a front desk these days! We are a professional organization now!

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +554,5 @@
>  
>      void moveValue(const Value &val, const ValueOperand &dest);
>  
>      void storeValue(ValueOperand val, Operand dst);
> +    void storeValue(ValueOperand val, Register base, Register index, int32 shift = 3);

:hardcoded_constants
Comment 2 Marty Rosenberg [:mjrosenb] 2012-01-06 01:29:27 PST
(In reply to Sean Stangl from comment #1)
> Comment on attachment 585546 [details] [diff] [review]
> implement the function LoadStoreElement functions
> 
> Review of attachment 585546 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This patch seems OK. I would prefer that archUseOrConstant() not exist,
> since it doesn't map nicely to a sensible abstraction and is extremely
> specific in the first place.
We talked about this on IRC, and decided that useArchOperand() is a more meaningful name, and fits the context better.

> I obviously don't know ARM well, so I looked at those sections only on the
> surface, assuming that operations don't have any tricky semantics. Most
> feedback is about commenting style and whitespace.
Nothing too complex, just a couple of places where the code could be made clearer by taking an extra temp
> 
> ::: js/src/ion/arm/Assembler-arm.h
> @@ +1006,5 @@
> >          *dest = Imm32(offset);
> >      }
> > +    Address toAddress() const {
> > +        return Address(Register::FromCode(reg), offset);
> > +    }
> 
> It shouldn't be necessary to construct an Address from an Operand. They are
> not used interchangeably.
> 
Consider it a temporary measure until I replace nearly every instance of Operand with Address, and transform Operand into something more useful than an alias for Address with more corner cases?

> @@ +729,5 @@
> >  {
> >      ma_dtr(IsStore, rt, addr, mode, cc);
> >  }
> > +void
> > +MacroAssemblerARM::ma_strd(Register rt, DebugOnly<Register> rt2, EDtrAddr addr, Index mode, Condition cc)
> 
> Having DebugOnly arguments is strange. If rt2 isn't explicitly used by
> as_extdtr, what effect does its value have?
You ran into this one earlier. strd and ldrd are parameterized on a single register, but access two registers, so
ldrd(r0, EDtrAddr(sp, 0)) will overwrite both r0 and r1 with data from the stack.  The extra parameter makes it obvious that the second register is being read/written.  Since rt2 is also sanity checked (must be one higher than rt), it is used in debug builds, but not in optimized builds.
> @@ +1448,5 @@
> > +void
> > +MacroAssemblerARMCompat::storeValue(ValueOperand val, Register base, Register index, int32 shift)
> > +{
> > +    
> > +    if (((val.payloadReg().code() & 1) == 0) &&
> 
> It would be great if "1" had a name. What does it mean to have the LSB set?
>
ldrd and strd take "two" registers, they must be adjacent, with the lower register even.  I've factored out this check into a function
isValueDTRDCandidate 

> @@ +1607,5 @@
> > +MacroAssemblerARMCompat::storePayload(Register src, Register base, Register index, int32 shift) {
> > +    JS_ASSERT(shift < 32);
> > +    JS_ASSERT(shift >= 0);
> > +    JS_STATIC_ASSERT(NUNBOX32_PAYLOAD_OFFSET == 0);
> > +    // technically, we can handle shift > -32 by changing LSL to ASR, but *effort*
> 
> 32, not -32, right? This comment could use a bit of professionalization. Not
> to keep harping on the comments :)
>
Nope, -32, if you want to interpret |x << -3| as |x >> 3|.  My biggest question is why this would ever be useful.  This is why it is unimplemented.
> @@ +1627,5 @@
> >  void
> > +MacroAssemblerARMCompat::storeTypeTag(ImmTag tag, Register base, Register index, int32 shift) {
> > +    JS_ASSERT(base != ScratchRegister);
> > +    JS_ASSERT(index != ScratchRegister);
> > +    // we need to store a value int base + index << shift + 4.  HOW AWKWARD
> 
> We have a front desk these days! We are a professional organization now!
> 
HOW AWKWARD
Comment 3 David Anderson [:dvander] 2012-01-06 02:13:33 PST
useArchOperand is really vague as to what it does and when it should be used. I'd suggest a name like "useAnyOrConstant" or "useAny" with some explanation that whether "any" includes stack slots or not is arch-specific.
Comment 4 Jan de Mooij [:jandem] 2012-01-06 02:31:16 PST
Just a heads-up, StoreElementT will need the same fix as in bug 715088 to pass ion/setelem.js (I added a new test there)
Comment 5 Nicolas B. Pierron [:nbp] 2012-01-08 18:17:48 PST
https://hg.mozilla.org/projects/ionmonkey/rev/2f73f3274b26

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