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

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dan Gohman [:sunfish]]

Attachment: 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).

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[Luke Wagner [:luke]]

Aren't we planning to remove LSRA?  Perhaps you could fix the LSRA bug nbp?

Comment 3

[Nicolas B. Pierron [: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

[Luke Wagner [:luke]]

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.

Comment 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[Dan Gohman [:sunfish]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6858829094

Comment 8

[Dan Gohman [:sunfish]]

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.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d6858829094