Closed
Bug 1074867
Opened 10 years ago
Closed 10 years ago
IonMonkey SIMD: move some operation lowering into the backends.
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
DUPLICATE
of bug 1074102
People
(Reporter: dougc, Assigned: dougc)
Details
Attachments
(2 files, 1 obsolete file)
10.16 KB,
patch
|
dougc
:
review+
|
Details | Diff | Splinter Review |
7.38 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
The SimdInsertElement and SimdBinaryComp Lowering is expected to become backend specific - it is my patch queue. Can we move them now.
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8497528 -
Flags: review?(benj)
Comment 2•10 years ago
|
||
Comment on attachment 8497528 [details] [diff] [review] Move some operation lowering into the backends. Review of attachment 8497528 [details] [diff] [review]: ----------------------------------------------------------------- While I agree for SimdInsertElement (which reuses its input), regarding SimdBinaryComp, I see that ARM also has a lowerForFPU method. Couldn't it be used in this case too? r+ if the answer is yes, otherwise let's discuss and please reflag for review. (it looks nice to keep stuff independent from the backend) ::: js/src/jit/shared/Lowering-x86-shared.cpp @@ +391,5 @@ > + > +bool > +LIRGeneratorX86Shared::visitSimdInsertElement(MSimdInsertElement *ins) > +{ > + JS_ASSERT(IsSimdType(ins->type())); please change this one to MOZ_ASSERT, while you're here
Attachment #8497528 -
Flags: review?(benj) → review+
Assignee | ||
Comment 3•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #2) > Comment on attachment 8497528 [details] [diff] [review] > Move some operation lowering into the backends. > > Review of attachment 8497528 [details] [diff] [review]: > ----------------------------------------------------------------- > > While I agree for SimdInsertElement (which reuses its input), regarding > SimdBinaryComp, I see that ARM also has a lowerForFPU method. Couldn't it be > used in this case too? r+ if the answer is yes, otherwise let's discuss and > please reflag for review. (it looks nice to keep stuff independent from the > backend) For SimdBinaryComp Lowering the x86 backend differs - it is better to swap the arguments at lowering to implement the integer less-than operation. I have this in the VEX patches but perhaps should split it out. Is that enough of a reason to move SimdBinaryComp too now?
Flags: needinfo?(benj)
Assignee | ||
Comment 5•10 years ago
|
||
Rebase. Carry forward r+. Try run: https://tbpl.mozilla.org/?tree=Try&rev=0362f6f29bf9
Attachment #8497528 -
Attachment is obsolete: true
Attachment #8498726 -
Flags: review+
Assignee | ||
Comment 6•10 years ago
|
||
Could we move this one too now? For the x86/x64 with VEX the lowering will differ, and it will differ for the ARM too.
Attachment #8498839 -
Flags: review?(benj)
Assignee | ||
Updated•10 years ago
|
Attachment #8498726 -
Attachment description: Move some operation lowering into the backends. → 1. Move some operation lowering into the backends.
Assignee | ||
Comment 7•10 years ago
|
||
Some failures in the try run, but they look like infrastructure issues. Requesting patch 1 land.
Keywords: checkin-needed,
leave-open
Assignee | ||
Comment 8•10 years ago
|
||
This might conflict with other patches landing, so shall wait and check and rebase if necessary.
Keywords: checkin-needed
Comment 9•10 years ago
|
||
Comment on attachment 8498839 [details] [diff] [review] 2. Move the shift operation lowering into the backends. Review of attachment 8498839 [details] [diff] [review]: ----------------------------------------------------------------- Although I fully understand the need for such code moves, it seems that it would be clearer to have these patches in bugs related to their uses (ex. VEX bug, ARM bug, etc.). In the meanwhile, I would prefer to keep the code as it is right now. Sorry for not have realized that yesterday. If you agree, would you be okay to move these patches in the bugs where we make use of them, and close this one instead? I'd rather not land these patches here. (you can keep the r+ though)
Attachment #8498839 -
Flags: review?(benj) → review+
Assignee | ||
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•