Re-enable integer divide-by-constant optimization with the backtracking allocator

RESOLVED FIXED in mozilla35

Status

()

Core
JavaScript Engine: JIT
--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

unspecified
mozilla35
x86_64
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

4 years ago
Created attachment 8478338 [details] [diff] [review]
reenable-div-constant-no-lsra.patch

This patch re-enables the integer divide-by-constant feature (bug 976110) when the register allocator is not LSRA (see also bug 1011283).
Attachment #8478338 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8478338 [details] [diff] [review]
reenable-div-constant-no-lsra.patch

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

I still don't think this is the right approach, see Bug 1011283 comment 15.
I do not see why we would need such condition, and why we could not improve LSRA to support this feature instead of working around it.
Attachment #8478338 - Flags: review?(nicolas.b.pierron)

Comment 2

4 years ago
Aren't we planning to remove LSRA?  Perhaps you could fix the LSRA bug nbp?
(In reply to Luke Wagner [:luke] from comment #2)
> Aren't we planning to remove LSRA?

Not yet, but I do not see any reasons for specializing the CodeGenerator on the register allocator.  These are just two phases are only supposed to interact with each others by the mean of LAllocation.  Having to add such check in the CodeGenerator highlight that somebody is going wrong.

So we should be fixing this other thing instead of working around it.

Is this optimization a blocker for anything?

Comment 4

4 years ago
Well, the reason is that LSRA is broken and it's apparently difficult to understand enough to fix and this is holding back an optimization that hurts us on awfy asmjs-ubench.  This patch is inherently a temporary measure, so objecting on aesthetics doesn't seem reasonable unless you want to fix LSRA.
Flags: needinfo?(jdemooij)
Comment on attachment 8478338 [details] [diff] [review]
reenable-div-constant-no-lsra.patch

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

I'll r+, but we shouldn't do this in other places and really need to make progress on enabling BT everywhere.
Attachment #8478338 - Flags: review+

Updated

4 years ago
Flags: needinfo?(jdemooij)
(Assignee)

Comment 8

4 years ago
asmjs-memops-ubench regressed on awfy because the register allocator is now putting a reload in the hot loop. I filed bug 1067610 to track this.
https://hg.mozilla.org/mozilla-central/rev/1d6858829094
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.