701993 - IonMonkey: Implement LLoadSlotT on ARM

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

These were added in bug 700303.

Comment 1

[Jan de Mooij [:jandem]]

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.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Patch

LoadSlotV, StoreSlotV and StoreSlotT were already implemented, so this just adds LoadSlotT.

Comment 3

[Marty Rosenberg [:mjrosenb]]

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.

Comment 4

[Jan de Mooij [:jandem]]

(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.

Comment 5

[Jan de Mooij [:jandem]]

http://hg.mozilla.org/projects/ionmonkey/rev/e43d4d0a2dcb