Closed Bug 1132466 Opened 10 years ago Closed 10 years ago

IonMonkey: Inline SIMD.int32x4.greaterThan calls

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1134638

People

(Reporter: andy.zsshen, Unassigned, Mentored)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36
Mentor: nicolas.b.pierron
Component: Untriaged → JavaScript Engine: JIT
Product: Firefox → Core
Attached patch SIMD_int32x4_Comp.patch (deleted) — Splinter Review
Try to inline the simd.int32x4.comp operations.
Attachment #8563505 - Flags: review?(nicolas.b.pierron)
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)
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)
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.

Attachment

General

Creator:
Created:
Updated:
Size: