Closed Bug 1071456 Opened 5 years ago Closed 5 years ago

IonMonkey: optimize some numeric conversions to useAtStart their argument.

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: dougc, Assigned: dougc)

References

Details

Attachments

(1 file, 4 obsolete files)

* The convertDoubleToFloat32() operation on the x86/x64 can useRegsterAtStart, as can the ARM and MIPS.

* The Int32ToDouble and Int32ToFloat32 operations can useRegsterAtStart on the ARM and MIPS.
Conversions between float widths are quite frequent in hot code so it might be worth optimizing them to useAtStart their argument. This relieves register pressure that tickled bug 1071403.

Would you want to move these rather large lowering methods into each backend or just conditionalize the code as this patch does?
Attachment #8493567 - Flags: feedback?(sunfish)
Comment on attachment 8493567 [details] [diff] [review]
Optimize some numeric conversions to useAtStart their argument

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

::: js/src/jit/Lowering.cpp
@@ +1797,2 @@
>          LInt32ToDouble *lir = new(alloc()) LInt32ToDouble(useRegister(opd));
> +#endif

x86 can actually use AtStart too. Even though it writes to its output before reading its input, the output is in a different and non-aliasing register class from the input (xmm vs. gpr), so writing to the output can't clobber the input.

@@ +1851,2 @@
>          LInt32ToFloat32 *lir = new(alloc()) LInt32ToFloat32(useRegister(opd));
> +#endif

As above.

Consequently, we don't need the ifdefs, and we can leave this as platform-independent code :-).
Attachment #8493567 - Flags: feedback?(sunfish)
(In reply to Dan Gohman [:sunfish] from comment #2)
> Comment on attachment 8493567 [details] [diff] [review]
> Optimize some numeric conversions to useAtStart their argument
> 
> Review of attachment 8493567 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/Lowering.cpp
> @@ +1797,2 @@
> >          LInt32ToDouble *lir = new(alloc()) LInt32ToDouble(useRegister(opd));
> > +#endif
> 
> x86 can actually use AtStart too. Even though it writes to its output before
> reading its input, the output is in a different and non-aliasing register
> class from the input (xmm vs. gpr), so writing to the output can't clobber
> the input.
I mentioned this on IRC, but wanted to get it in a slightly more permanent medium.  Using the same logic for why this is safe on x86, (outputs and inputs cannot possibly overlap), these changes should have no effect, since the primary reason for using useAtStart rather than use is that it enables the register allocator to re-assign an input as an output.
Reverting the changes to the int to float conversions as they seem pointless.
Attachment #8493567 - Attachment is obsolete: true
Attachment #8494237 - Flags: review?(sunfish)
It's true that there's no direct effect of AtStart on Int32ToDouble and related conversions. However, I still prefer that we should use AtStart for them. To me, AtStart should be the default choice, with non-AtStart indicating that the instruction has special needs.

Ideally, we should replace the AtStart flag with an AtEnd flag with the opposite meaning, but that's a bigger change :-).
Using useRegisterAtStart for the integer to float conversions looks good, thanks.
Attachment #8494237 - Attachment is obsolete: true
Attachment #8494237 - Flags: review?(sunfish)
Attachment #8495176 - Flags: review?(sunfish)
Bug 1039993 constrains useRegisterAtStart here for ARM and MIPS when using the LSRA. Unfortunate, but this seems to be the state of the register allocators.
Attachment #8495176 - Attachment is obsolete: true
Attachment #8495176 - Flags: review?(sunfish)
Attachment #8495228 - Flags: review?(sunfish)
Comment on attachment 8495228 [details] [diff] [review]
Optimize DoubleToFloat32 to useAtStart the argument

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

::: js/src/jit/Lowering.cpp
@@ +1837,5 @@
>        case MIRType_Double:
>        {
> +        LDoubleToFloat32 *lir;
> +#if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_MIPS)
> +        // Bug 1039993: workaround LSRA issues.

But 1039993 is marked fixed, though there's an outstanding patch, though it has an r+. When that patch lands, does that mean this workaround is no longer needed? If so, can we just wait for that patch and remove this workaround? If not, we should put a new comment here.
Attachment #8495228 - Flags: review?(sunfish)
Comment on attachment 8495228 [details] [diff] [review]
Optimize DoubleToFloat32 to useAtStart the argument

The patches in bug 1039993 do not fix the problems with LSRA. They just workaround them by avoiding useAtStart.

The pending patch in 1039993 fixes a stack alignment issue that caused tbpl jit-test failures on Windows and it also does not address the problems with LSRA and perhaps it should have been split out and could still be. This was worked around by keeping useAtStart for x86 and x64, but we still need to landed the alignment fix.

Thus the comment in this patch seems fine to me - the code in this patch includes a workaround for LSRA issues that have been discussed in bug 1039993. Happy to elaborate on the comment if you have a suggestion?

A new bug should probably have been filed to fix the LSRA issues given that bug 1039993 only works around them, but this was not done or noted in the patches already landed in bug 1039993.
Attachment #8495228 - Flags: review?(sunfish)
Comment on attachment 8495228 [details] [diff] [review]
Optimize DoubleToFloat32 to useAtStart the argument

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

Ah, ok. This is fine for now then. Thanks for being patient!
Attachment #8495228 - Flags: review?(sunfish) → review+
Try run failed on the ARM: https://tbpl.mozilla.org/?tree=Try&rev=c88943302c75

We'll need bug 1072881 resolved first.
Depends on: 1072881
Rebase after bug 1072881. Carrying forward r+.
Attachment #8495228 - Attachment is obsolete: true
Attachment #8495970 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0a90ee0dad24
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.