Last Comment Bug 740733 - IonMonkey: Implement GETELEM for typed arrays on ARM
: IonMonkey: Implement GETELEM for typed arrays 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-03-30 01:39 PDT by Marty Rosenberg [:mjrosenb]
Modified: 2012-07-30 08:55 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
/home/mrosenberg/patches/TypedArrayOnARM-r0.patch (36.96 KB, patch)
2012-03-30 01:39 PDT, Marty Rosenberg [:mjrosenb]
Jacob.Bramley: review+
Details | Diff | Review

Description Marty Rosenberg [:mjrosenb] 2012-03-30 01:39:23 PDT
Created attachment 610837 [details] [diff] [review]
/home/mrosenberg/patches/TypedArrayOnARM-r0.patch

port of basic functions to ARM.
the full range of loads is not yet supported, but it should be pretty trivial to add it.
I feel like a bunch of the code in ma_dataTransferN can be refactored (in addition to fixing its correctness).  For now, I should just assert that mode == offset whenever we compile to more than a single instruction
Comment 1 Jacob Bramley [:jbramley] 2012-03-30 07:54:06 PDT
Comment on attachment 610837 [details] [diff] [review]
/home/mrosenberg/patches/TypedArrayOnARM-r0.patch

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

A few minor issues, but it mostly looks good.

::: js/src/ion/IonMacroAssembler.cpp
@@ +243,5 @@
>          else
>              loadDouble(src, dest.fpu());
>  
> +#ifdef JS_CPU_ARM
> +        // this code won't get committed, it is waiting on a patch from jandem so I can just call branchCompareDoubles

In general, I'd prefer to see the final version. It's hard to review with these not-quite-final bits mixed in.

@@ +251,5 @@
> +        {
> +            loadStaticDouble(&js_NaN, dest.fpu());
> +        }
> +        bind(&notNaN);
> +#if 0 // or I could do this, which will likely be faster, particularly if we don't need to load 0.0

Don't leave this in.

@@ +254,5 @@
> +        bind(&notNaN);
> +#if 0 // or I could do this, which will likely be faster, particularly if we don't need to load 0.0
> +        // need to have a brief talk with other people to see if we can set default mode in main().
> +        ma_vimm(0.0, ScratchFloatReg);
> +        ma_vadd(ScratchFloatReg, dest.fpu(), dest.fpu());

I thought about this after we discussed it on IRC. I found a solution that uses just two instructions:

vmov.f64  out, #1.0
vmul.f64  out, out, in

I've got a few three-instruction forms too, and a faster two-instruction form that doesn't handle infinities correctly, but this multiplication-based solution seems to work for all inputs and it should be pretty fast.

It's really good to avoid the FP->integer transfer of the FPSCR.

::: js/src/ion/arm/Assembler-arm.cpp
@@ +3,5 @@
>   *
>   * ***** BEGIN LICENSE BLOCK *****
>   * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>   *
> + * THE contents of this file are subject to the Mozilla Public License Version

This looks unintentional.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +797,5 @@
>  MacroAssemblerARM::ma_dtr(LoadStore ls, Register rn, Imm32 offset, Register rt,
>                            Index mode, Assembler::Condition cc)
>  {
> +    ma_dataTransferN(ls, 32, true, rn, offset, rt, mode, cc);
> +#if 0

Don't leave this in. If the block should go, delete it.

@@ +835,5 @@
>  {
> +    ma_dataTransferN(ls, 32, true,
> +                     Register::FromCode(addr.base()), Imm32(addr.disp()),
> +                     rt, mode, cc);
> +#if 0

Don't leave this in.

@@ +945,5 @@
>  
>  void
>  MacroAssemblerARM::ma_dataTransferN(LoadStore ls, int size, bool IsSigned,
> +                                    Register rn, Imm32 offset, Register rt,
> +                                    Index mode, Assembler::Condition cc)

I can't see what's changed here. Is it a whitespace change that the splinter review has hidden?

@@ +1443,5 @@
> +
> +    if (src.offset != 0) {
> +        ma_mov(base, ScratchRegister);
> +        base = ScratchRegister;
> +        ma_add(Imm32(src.offset), base);

Can you use a three-operand form?

ma_add(Imm32(src.offset), base, ScratchRegister);

@@ +1460,5 @@
> +MacroAssemblerARMCompat::load8SignExtend(const BaseIndex &src, const Register &dest)
> +{
> +    Register index = src.index;
> +
> +    // We don't have LSL on index register yet.

Is there a TODO for this? (It sounds like there should be.)

@@ +1468,5 @@
> +    }
> +    if (src.offset != 0) {
> +        if (index != ScratchRegister) {
> +            ma_mov(index, ScratchRegister);
> +            index = ScratchRegister;

Again, a three-operand form would be nice.

@@ +1482,5 @@
>      ma_dataTransferN(IsLoad, 16, false, address.base, Imm32(address.offset), dest);
>  }
>  
>  void
> +MacroAssemblerARMCompat::load16ZeroExtend_mask(const Address &address, Imm32 mask, const Register &dest)

This is a strange sequence to lump into one method, isn't it?

@@ +1526,5 @@
> +        index = ScratchRegister;
> +    }
> +    if (src.offset != 0) {
> +        if (index != ScratchRegister) {
> +            ma_mov(index, ScratchRegister);

Three-operand form.

@@ +1994,5 @@
>  
>  void
> +MacroAssemblerARMCompat::boxDouble(const FloatRegister &src, const ValueOperand &dest)
> +{
> +    //JS_ASSERT(src != ScratchFloatReg);

Don't leave this commented out.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +885,5 @@
> +    void loadDouble(const BaseIndex &src, const FloatRegister &dest);
> +
> +    // Load a float value into a register, then expand it to a double.
> +    void loadFloat(const Address &addr, const FloatRegister &dest);
> +    void loadFloat(const BaseIndex &src, const FloatRegister &dest);

A name like "loadFloatAsDouble" might be clearer.
Comment 2 Marty Rosenberg [:mjrosenb] 2012-07-30 08:55:40 PDT
landed: http://hg.mozilla.org/projects/ionmonkey/rev/48a6ce8bd921

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