Last Comment Bug 738277 - IonMonkey: SETELEM: Fast path for typed arrays
: IonMonkey: SETELEM: Fast path for typed arrays
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
:
Mentors:
Depends on:
Blocks: 735408
  Show dependency treegraph
 
Reported: 2012-03-22 08:49 PDT by Jan de Mooij [:jandem]
Modified: 2012-03-31 11:57 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (51.47 KB, patch)
2012-03-22 09:13 PDT, Jan de Mooij [:jandem]
dvander: review+
Details | Diff | Splinter Review
Add DoubleCondition (21.80 KB, patch)
2012-03-26 07:43 PDT, Jan de Mooij [:jandem]
dvander: review+
marty.rosenberg: review+
Details | Diff | Splinter Review

Description Jan de Mooij [:jandem] 2012-03-22 08:49:22 PDT

    
Comment 1 Jan de Mooij [:jandem] 2012-03-22 09:13:25 PDT
Created attachment 608358 [details] [diff] [review]
Patch

I also wrote JM+TI's inline typed array paths, and Ion's type analysis pass and the separate instructions for conversion/clamping make things a lot easier here.

There's one line marked with XXX. It's correct, but I think it would be nicer to add Assembler::DoubleCondition or something there? I can do that in a follow-up patch.
Comment 2 David Anderson [:dvander] 2012-03-22 18:32:42 PDT
Comment on attachment 608358 [details] [diff] [review]
Patch

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

Great! r=me with one suggestion, to get rid of LClampVToUint8 if you think it's viable.

::: js/src/ion/TypePolicy.cpp
@@ +438,5 @@
> +        break;
> +      case TypedArray::TYPE_UINT8_CLAMPED:
> +        value = MClampToUint8::New(value);
> +        ins->block()->insertBefore(ins, value->toInstruction());
> +        break;

In the case of MClampToUint8, I think we should insert this in IonBuilder instead since we're always going to insert it.

That would also let MClampToUint8 have a policy which accepts Int|Double, and otherwise adds an MTruncateToInt32. Then we don't have to explicitly handle the V case in LIR. I'd do this unless it's actually worthwhile to have the untyped clamping cases completely inline.

::: js/src/ion/shared/Lowering-x86-shared.cpp
@@ +110,5 @@
> +    LAllocation value;
> +
> +    // For byte arrays, the value has to be in a byte register.
> +    if (ins->isByteArray())
> +        value = useFixed(ins->value(), eax);

Phew. This is a lot simpler than what JM had to do! Note I think this is only needed on x86; x64 can use any register as a byte register, as long as you have the REX prefix (which JM did emit).
Comment 3 Jan de Mooij [:jandem] 2012-03-23 01:02:38 PDT
(In reply to David Anderson [:dvander] from comment #2)
>
> That would also let MClampToUint8 have a policy which accepts Int|Double,
> and otherwise adds an MTruncateToInt32.

In some edge cases clamping behaves differently, e.g. MTruncateToInt32(Infinity) is 0, while clamping Infinity results in 255. MToDouble would work (as long as it doesn't handle objects), but we would have to convert int32 values to double, then clamp the double instead of clamping the integer directly. I think it's worthwhile to keep the untyped version since int32 is probably the most common type here.

> 
> Note I think this is
> only needed on x86; x64 can use any register as a byte register, as long as
> you have the REX prefix (which JM did emit).

Interesting. I will have a look, it would really help regalloc.

Thanks for the quick review!
Comment 4 Jan de Mooij [:jandem] 2012-03-26 07:43:20 PDT
Created attachment 609319 [details] [diff] [review]
Add DoubleCondition

Here's the follow-up patch I mentioned in comment 1. This adds a DoubleCondition enum so that we can use things like DoubleEqual, DoubleLessThanOrUnordered etc in platform-independent code.

The main problem here is that not all of these conditions map to a single x86/ARM flag. The patch handles this like JSC's assembler, an extra bit is set for these conditions and the macro-assembler inserts two jumps in this case. I couldn't think of a better approach and this works pretty well.

Flagging Marty for the ARM changes.
Comment 5 David Anderson [:dvander] 2012-03-26 18:46:32 PDT
Comment on attachment 609319 [details] [diff] [review]
Add DoubleCondition

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

Cool. this looks a little cleaner

::: js/src/ion/IonMacroAssembler.cpp
@@ +357,5 @@
>      branch32(Assembler::Above, output, Imm32(255), &outOfRange);
>      {
>          // Check if we had a tie.
>          convertInt32ToDouble(output, ScratchFloatReg);
> +        branchDouble(Assembler::DoubleNotEqual, input, ScratchFloatReg, &done);

No need for Assembler:: here, I think (and above)
Comment 6 Marty Rosenberg [:mjrosenb] 2012-03-28 11:37:55 PDT
Comment on attachment 609319 [details] [diff] [review]
Add DoubleCondition

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

Nothing wrong here, just slightly different from JM's implementation.  There is nothing wrong with it, but we should keep it in mind for when we have a more uniformly fast engine, so small differences in code generation can be reliably measured.

::: js/src/ion/arm/MacroAssembler-arm.cpp
@@ +1477,5 @@
> +
> +    if (cond == DoubleNotEqual) {
> +        // Force the unordered cases not to jump.
> +        Label unordered;
> +        ma_b(&unordered, VFP_Unordered);

I believe the way this was handled in JM was |cmpvs r0, r0|, which only executes when the overflow (or unordered in VFP-speak), then is guaranteed to set the zero flag, which will be interpreted as "equal", meaning the next branch won't be taken.

@@ +1483,5 @@
> +        bind(&unordered);
> +        return;
> +    }
> +    if (cond == DoubleEqualOrUnordered) {
> +        ma_b(label, VFP_Unordered);

Similarly, you should be able to emit a |cmpvs r0, r0| here, which will only execute in the unordered case, and set the zero/equal flag guaranteeing the ma_b(label, VFP_Equal) branch is taken.

::: js/src/ion/arm/MacroAssembler-arm.h
@@ +885,5 @@
>      void setStackArg(const Register &reg, uint32 arg);
>  
>      void breakpoint();
>  
> +    void compareDouble(DoubleCondition cond, FloatRegister lhs, FloatRegister rhs);

It looks like compareDouble is now a function that *requires* post processing (namely from branchDouble).  If this is the case, it should be marked private.
Comment 7 Jan de Mooij [:jandem] 2012-03-31 11:57:50 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/196cd36978ba
https://hg.mozilla.org/projects/ionmonkey/rev/5534be14e707

This was orange on TBPL because one of the tests I added fails with -m (but not -m -n). I filed bug 741114 and disabled the testcase in the meantime:

https://hg.mozilla.org/projects/ionmonkey/rev/fe58c6671ebd

(In reply to Marty Rosenberg [:mjrosenb] from comment #6)
> 
> It looks like compareDouble is now a function that *requires* post
> processing (namely from branchDouble).  If this is the case, it should be
> marked private.

compareDouble is also called by CodeGeneratorARM so I had to leave it public.

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