IonMonkey ARM: optimize Float32ToDouble to useAtStart its argument.

RESOLVED FIXED in mozilla35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dougc, Assigned: dougc)

Tracking

unspecified
mozilla35
ARM
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

5 years ago
Bug 1039993 modified Float32ToDouble on the ARM and MIPS to not useAtStart its argument due to LSRA issues, however it can still be optimized to useAtStart its argument when not using the LSRA and this will help asm.js code which often has a lot of float32 to double conversions.
Assignee

Comment 1

5 years ago
We run into the LSRA issues from bug 1039993, so the code dynamically avoids useAtStart for the when using the LSRA.

Updated the debug checks added in bug 1039993 to only apply when using the LSRA.

Bug 1039993 appears to have dropped a necessary check which has been restored:
JS_ASSERT(!(hasUseRegister && hasUseRegisterAtStart));
Attachment #8495231 - Flags: review?(mrosenberg)
Comment on attachment 8495231 [details] [diff] [review]
Optimize Float32ToDouble to useAtStart its argument.

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

::: js/src/jit/LiveRangeAllocator.cpp
@@ +805,2 @@
>  
> +# if defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_MIPS)

This should be unnecessary, and possibly wrong. hasUnaliasedDouble is only true on ARM, and only some ARM chips at that.
The hasMultiAlias should also protect the rest of the cases as well.

::: js/src/jit/Lowering.cpp
@@ +1784,5 @@
>          // should still be, however, there is a bug in LSRA's implementation of
>          // *AtStart, which is quite fundamental. This should be reverted when that
>          // is fixed, or lsra is deprecated.
> +        if (gen->optimizationInfo().registerAllocator() == RegisterAllocator_LSRA)
> +            lir = new(alloc()) LFloat32ToDouble(useRegister(opd));

nit: new (alloc()), here and below.
Attachment #8495231 - Flags: review?(mrosenberg) → review+
Assignee

Updated

5 years ago
Blocks: 1071456
Assignee

Comment 3

5 years ago
Thank you for the quick review. Re-enabled the assertions on all backends, thanks. Carrying forward r+. Try run:
https://tbpl.mozilla.org/?tree=Try&rev=b89f0ca2f6b0
Attachment #8495231 - Attachment is obsolete: true
Attachment #8495966 - Flags: review+
Assignee

Comment 4

5 years ago
Some orange in the try run in comment 3, but it does not appear related to this patch.

Here's another try build that included this patch and while it has some different orange and red it does not appear related: https://tbpl.mozilla.org/?tree=Try&rev=eb54d95aacba
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/22b5ded1d700
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.