Closed
Bug 1132466
Opened 10 years ago
Closed 10 years ago
IonMonkey: Inline SIMD.int32x4.greaterThan calls
Categories
(Core :: JavaScript Engine: JIT, defect)
Tracking
()
RESOLVED
DUPLICATE
of bug 1134638
People
(Reporter: andy.zsshen, Unassigned, Mentored)
Details
Attachments
(1 file)
(deleted),
patch
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36
Reporter | ||
Updated•10 years ago
|
Mentor: nicolas.b.pierron
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Reporter | ||
Comment 1•10 years ago
|
||
Try to inline the simd.int32x4.comp operations.
Attachment #8563505 -
Flags: review?(nicolas.b.pierron)
Comment 2•10 years ago
|
||
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.
Attachment #8563505 -
Flags: review?(nicolas.b.pierron)
Reporter | ||
Comment 3•10 years ago
|
||
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?
Flags: needinfo?(nicolas.b.pierron)
Comment 4•10 years ago
|
||
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.
Status: UNCONFIRMED → RESOLVED
Closed: 10 years ago
Flags: needinfo?(nicolas.b.pierron)
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•