1132466 - IonMonkey: Inline SIMD.int32x4.greaterThan calls

Status: RESOLVED DUPLICATE of bug 1134638

(Core :: JavaScript Engine: JIT, defect)

Description

[andy.zsshen]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36

Comment 1

[andy.zsshen]

Attachment: SIMD_int32x4_Comp.patch

Try to inline the simd.int32x4.comp operations.

Comment 2

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8563505 [details] [diff] [review]
SIMD_int32x4_Comp.patch

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

::: js/src/builtin/SIMD.h
@@ +222,4 @@
>  #define FOREACH_COMMONX4_SIMD_OP(_)  \
>      ARITH_COMMONX4_SIMD_OP(_)        \
>      BITWISE_COMMONX4_SIMD_OP(_)      \
>      _(lessThan)                      \

Can you use the Macro here as well?

::: js/src/jit/MCallOptimize.cpp
@@ +2820,5 @@
> +
> +    // If the type of any of the arguments is neither a SIMD type, an Object
> +    // type, or a Value, then the applyTypes phase will add a fallible box &
> +    // unbox sequence.  This does not matter much as the binary bitwise
> +    // instruction is supposed to produce a TypeError once it is called.

This comment is not accurate as the return type of MSimdBinaryComp is always an Int32x4.  So we might have to collect extra information to be able to select the correct variant of the compare function.

::: js/src/jit/MIR.h
@@ +1924,5 @@
>  
> +    static MSimdBinaryComp *New(TempAllocator &alloc, MDefinition *left, MDefinition *right,
> +                                Operation op)
> +    {
> +        return new(alloc) MSimdBinaryComp(left, right, op);

Unfortunately this would not work as expected, can you add a test case which make this fail?
Also you will see that some part of the conde in the constructor will not work as expected, and that MSimdBinaryComp requires a new type policy which is used by Ion to coerce the input of the instruction into the expected types.

Comment 3

[andy.zsshen]

Sorry for the delayed update. I am back to work and have some problems about the implementation of
the type policy.

There are two assertions in the constructor of MSimdBinaryComp:
    MOZ_ASSERT(IsSimdType(left->type()));
    MOZ_ASSERT(left->type() == right->type());

It seems that MSimdBinaryComp only allows its operands to be SIMD vectors instead of SIMD objects.
And the operands should also have the same SIMD vector type.
Should we relax these restrictions and let the type analyzer to handle the inconsistent operands?

Comment 4

[Nicolas B. Pierron [:nbp]]

I am sorry, Benjamin made a patch in Bug 1134638 (part 10) which solve this issue.

(In reply to andy.zsshen from comment #3)
> Sorry for the delayed update. I am back to work and have some problems about
> the implementation of
> the type policy.
> 
> There are two assertions in the constructor of MSimdBinaryComp:
>     MOZ_ASSERT(IsSimdType(left->type()));
>     MOZ_ASSERT(left->type() == right->type());

Yeah, these should have been moved to another function called NewAsmJS, and imply that we add a "New" function that is used in MCallOptimize.

> It seems that MSimdBinaryComp only allows its operands to be SIMD vectors
> instead of SIMD objects.
> And the operands should also have the same SIMD vector type.
> Should we relax these restrictions and let the type analyzer to handle the
> inconsistent operands?

Yes, this is the idea, but only for Ion.

Feel free to ping Benjamin on IRC (bbouvier) to know if he can find Simd operations which are not as urgent to implement.