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)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1074102

People

(Reporter: dougc, Assigned: dougc)

Details

Attachments

(2 files, 1 obsolete file)

The SimdInsertElement and SimdBinaryComp Lowering is expected to become backend specific - it is my patch queue. Can we move them now.
Attachment #8497528 - Flags: review?(benj)
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+
(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)
Yes, sounds good to me.
Flags: needinfo?(benj)
Rebase. Carry forward r+. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=0362f6f29bf9
Attachment #8497528 - Attachment is obsolete: true
Attachment #8498726 - Flags: review+
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)
Attachment #8498726 - Attachment description: Move some operation lowering into the backends. → 1. Move some operation lowering into the backends.
Some failures in the try run, but they look like infrastructure issues. Requesting patch 1 land.
This might conflict with other patches landing, so shall wait and check and rebase if necessary.
Keywords: checkin-needed
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+
Status: NEW → RESOLVED
Closed: 10 years ago
Keywords: leave-open
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: