Last Comment Bug 701993 - IonMonkey: Implement LLoadSlotT on ARM
: IonMonkey: Implement LLoadSlotT on ARM
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: ARM All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on: 700303 708063
Blocks: IonMonkey
  Show dependency treegraph
 
Reported: 2011-11-12 01:02 PST by Jan de Mooij [:jandem]
Modified: 2011-12-19 00:58 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.98 KB, patch)
2011-12-10 02:10 PST, Jan de Mooij [:jandem]
marty.rosenberg: review+
Details | Diff | Review

Description Jan de Mooij [:jandem] 2011-11-12 01:02:51 PST
These were added in bug 700303.
Comment 1 Jan de Mooij [:jandem] 2011-12-02 10:46:32 PST
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] 2011-12-10 02:10:53 PST
Created attachment 580629 [details] [diff] [review]
Patch

LoadSlotV, StoreSlotV and StoreSlotT were already implemented, so this just adds LoadSlotT.
Comment 3 Marty Rosenberg [:mjrosenb] 2011-12-12 17:50:32 PST
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] 2011-12-13 01:33:43 PST
(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] 2011-12-19 00:58:35 PST
http://hg.mozilla.org/projects/ionmonkey/rev/e43d4d0a2dcb

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