Closed Bug 701993 Opened 13 years ago Closed 12 years ago

IonMonkey: Implement LLoadSlotT on ARM

Categories

(Core :: JavaScript Engine, defect)

ARM
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jandem, Assigned: jandem)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

These were added in bug 700303.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
In addition to LStoreSlot{V,T}, we also need to add LStoreElement{V,T} and LLoadElement{V,T}. I'll get to this next week.
Depends on: 708063
Attached patch PatchSplinter Review
LoadSlotV, StoreSlotV and StoreSlotT were already implemented, so this just adds LoadSlotT.
Attachment #580629 - Flags: review?(mrosenberg)
Summary: IonMonkey: Implement LStoreSlotV and LStoreSlotT on ARM → IonMonkey: Implement LLoadSlotT on ARM
Comment on attachment 580629 [details] [diff] [review]
Patch

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

::: js/src/ion/arm/CodeGenerator-arm.cpp
@@ +1290,5 @@
>      Register base = ToRegister(load->input());
>      int32 offset = load->mir()->slot() * sizeof(js::Value);
>  
>      if (load->mir()->type() == MIRType_Double)
> +        masm.loadInt32OrDouble(Operand(base, offset), ToFloatRegister(load->output()));

When mir()->type() is MIRType_Double, can we really get a value that is an integer?

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +988,5 @@
>  
>  void
>  MacroAssemblerARM::ma_vstr(FloatRegister src, const Operand &addr)
>  {
> +    ma_vdtr(IsStore, addr, src);

I didn't already do that? whoops.

@@ +1224,5 @@
> +    Label notInt32, end;
> +
> +    // If it's an int, convert it to double.
> +    ma_ldr(ToType(src), ScratchRegister);
> +    branchTestInt32(Assembler::NotEqual, ScratchRegister, &notInt32);

Are we sure the only two tags we'll ever see here are int and double?

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +488,5 @@
>      }
>  
>      void boolValueToDouble(const ValueOperand &operand, const FloatRegister &dest);
>      void int32ValueToDouble(const ValueOperand &operand, const FloatRegister &dest);
> +    void loadInt32OrDouble(const Operand &src, const FloatRegister &dest);

Do we already use the name "loadInt32OrDouble" on other architectures?  It just seems like it could be better named.
Attachment #580629 - Flags: review?(mrosenberg) → review+
(In reply to Marty Rosenberg [:Marty] from comment #3)
> 
> When mir()->type() is MIRType_Double, can we really get a value that is an
> integer?

Yeah this information comes from TI and there the double type includes the int32 type. When storing an integer to a known-double slot we don't convert it to double so we have to handle integers when loading a known-double from memory. JM has similar code in place and see bug 705351 comment 0 for a testcase where this matters.

> 
> Are we sure the only two tags we'll ever see here are int and double?

Yes, when a value of another type is stored, TI/OSI will invalidate the script.

> 
> Do we already use the name "loadInt32OrDouble" on other architectures?  It
> just seems like it could be better named.

We use the name on x86/x64 too. Let me know if you have a better name and I can change it.
http://hg.mozilla.org/projects/ionmonkey/rev/e43d4d0a2dcb
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.