886960 - IonMonkey: Use discarded headers when rebuilding inner loops

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

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

Comment 1

[Jan de Mooij [:jandem]]

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?

Comment 2

[Brian Hackett [Laid off!]]

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

Comment 3

[Guilherme Lima]

This patch regressed asmjs-apps-bullet-workload* on AWFY

Comment 4

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8e5008ac7f1

Comment 5

[Brian Hackett [Laid off!]]

(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).