IonMonkey: Use discarded headers when rebuilding inner loops

RESOLVED FIXED in mozilla25

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: bhackett, Unassigned)

Tracking

(Blocks: 1 bug)

unspecified
mozilla25
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 767387 [details] [diff] [review]
patch

In IonBuilder, if the loop backedge is reached and new types are found for any phis at the header then processing of the loop is restarted.  If the loop contains any inner loops, this throws away all type information discovered for those loops, and if they themselves needed to be restarted then they may need to do so repeatedly, discovering the same 'new' information over and over.  When loops are deeply nested this becomes a bit of a mess.

There is a large, critical script in the asm.js box2d benchmark that restarts loop processing over 60 times, hitting the threshold where we mark a script as uncompilable after 20 restarts.

The attached patch keeps track of the most recent loop header for each loop in the processed script, and pulls in types from any discarded header when starting a new loop.  This decreases the number of restarts on the above script to 19, just squeaking under the above threshold.  The patch also doubles this threshold as many restarts in such large scripts (this one has a bytecode length of 47k) doesn't necessarily indicate a problem.

In combination with bug 886957 this makes some of the box2d asm.js benchmarks run about 3x faster when using --no-asmjs.
Attachment #767387 - Flags: review?(jdemooij)
Comment on attachment 767387 [details] [diff] [review]
patch

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

::: js/src/ion/IonBuilder.cpp
@@ +337,5 @@
> +            {
> +                MPhi *newPhi = entry->getSlot(oldPhi->slot())->toPhi();
> +                newPhi->addBackedgeType(oldPhi->type(), oldPhi->resultTypeSet());
> +            }
> +            return;

As discussed on IRC, add a |loopHeaders_[i].header = entry| here.

@@ +340,5 @@
> +            }
> +            return;
> +        }
> +    }
> +    loopHeaders_.append(LoopHeader(start, entry));

OOM check?
Attachment #767387 - Flags: review?(jdemooij) → review+
(Reporter)

Comment 2

4 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8e5008ac7f1

Comment 3

4 years ago
This patch regressed asmjs-apps-bullet-workload* on AWFY
https://hg.mozilla.org/mozilla-central/rev/e8e5008ac7f1
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
(Reporter)

Comment 5

4 years ago
(In reply to Guilherme Lima from comment #3)
> This patch regressed asmjs-apps-bullet-workload* on AWFY

This patch allows Ion compilation to be used more often on asm.js workloads and probably exposed some preexisting issues.  I'll look at this, though maybe not for a week or so (working on some parallelization stuff).
You need to log in before you can comment on or make changes to this bug.