Last Comment Bug 732120 - IonMonkey crashes on box2d benchmark
: IonMonkey crashes on box2d benchmark
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
Mentors:
Depends on:
Blocks: 732861
  Show dependency treegraph
 
Reported: 2012-03-01 11:44 PST by Alon Zakai (:azakai)
Modified: 2012-03-21 05:26 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
rm ReorderBlocks (17.53 KB, patch)
2012-03-02 06:29 PST, Jan de Mooij [:jandem]
sstangl: review+
Details | Diff | Splinter Review

Description Alon Zakai (:azakai) 2012-03-01 11:44:39 PST
A box2d benchmark appears here,

http://hg.mozilla.org/users/bhackett_mozilla.com/awfy-assorted/file/bf803819ac12/tests/assorted/misc-box2d.js

IonMonkey crashes on it, which prevents it from being added to AWFY.
Comment 1 Jan de Mooij [:jandem] 2012-03-02 06:29:01 PST
Created attachment 602339 [details] [diff] [review]
rm ReorderBlocks

This patch ensures IonBuilder and critical edge splitting insert blocks in the correct order, so that we no longer need ReorderBlocks. This approach is a lot simpler and removes 130 buggy and complicated lines of code. The patch asserts the graph is in RPO and passes jit-tests with "--ion -n" and "--ion-eager -n".

box2d no longer crashes with this patch - performance is (slightly) better than the interpreter, even though we don't have fast paths for typed arrays and are unable to compile some functions (unsupported opcodes, call objects etc).

Alon, since AWFY runs all tests at least 10 times, it's probably a good idea to modify the script so that it doesn't take more than a few seconds. On my laptop, -m -n takes 7 seconds, --ion -n 140 seconds and the interpreter 175 seconds.
Comment 2 Jan de Mooij [:jandem] 2012-03-02 07:02:52 PST
(In reply to Jan de Mooij (:jandem) from comment #1)
> 
> On my
> laptop, -m -n takes 7 seconds, --ion -n 140 seconds and the interpreter 175
> seconds.

(110 seconds after updating to tip, progress!)
Comment 3 Alon Zakai (:azakai) 2012-03-02 10:35:49 PST
I can make the test shorter, but I fear it will at some point become meaningless, since it takes time to set up the data to be processed. How much faster should I make it?
Comment 4 Alon Zakai (:azakai) 2012-03-02 11:58:09 PST
I made the benchmark a little more than 2x faster just now, but I'm not sure I can do much more. So it might be 50 seconds or so in ion now. Is that still too slow?
Comment 5 Jan de Mooij [:jandem] 2012-03-05 13:27:11 PST
Comment on attachment 602339 [details] [diff] [review]
rm ReorderBlocks

Changing reviewer per our IRC discussion.

ReorderBlocks tries hard to match loop headers and backedges, to prevent situations where blocks are ordered like this:

<header 1>
<header 2>
<backedge 1>
<backedge 2>

The reason is that such an ordering makes greedy's FindNaturalLoops and alias analysis unhappy, because they assume inner backedges are visited before outer backedges.

However, the code to do this in ReorderBlocks is a real mess and error-prone (as this bug illustrates). So the idea is to have IonBuilder and critical edge splitting insert blocks in the correct order so that we don't need to reorder them later. This should also allow us to remove some LSRA code to handle non-contiguous loop blocks.

At first I tried to create the successor block, and add it when we're done with the loop/if-else body. However, this broke setBackedge, which wants the block to have an id > the loop header id. So this patch instead adds the block, then moves it when we're done processing the body.
Comment 6 Jan de Mooij [:jandem] 2012-03-05 13:42:26 PST
(In reply to Alon Zakai (:azakai) from comment #4)
> I made the benchmark a little more than 2x faster just now, but I'm not sure
> I can do much more. So it might be 50 seconds or so in ion now. Is that
> still too slow?

I'm not sure - maybe it does not really matter, especially once we have fast paths for typed arrays. We can just try though and if it makes AWFY too slow we can reduce the number of runs or change the harness to run tests marked as slow only once, or something.
Comment 7 Jan de Mooij [:jandem] 2012-03-13 01:35:24 PDT
Review ping.
Comment 8 Alon Zakai (:azakai) 2012-03-20 14:32:57 PDT
Ping from me as well. This bug is preventing testing of IonMonkey on Emscripten-generated code.
Comment 9 Sean Stangl [:sstangl] 2012-03-20 14:42:30 PDT
Comment on attachment 602339 [details] [diff] [review]
rm ReorderBlocks

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

As discussed on IRC, I am not convinced that the IonBuilder behavior necessarily guarantees correct ordering -- but we can address fallout after landing. Deviations should hopefully be small.

::: js/src/ion/Ion.cpp
@@ +651,3 @@
>      AssertGraphCoherency(graph);
>  
> +    AssertReversePostOrder(graph);

We can move AssertReversePostOrder() into AssertGraphCoherency() so that it is called after every phase that changes the MIRGraph. The graph should always be in RPO.
Comment 10 Jan de Mooij [:jandem] 2012-03-21 05:26:00 PDT
Pushed with nit fixed.

https://hg.mozilla.org/projects/ionmonkey/rev/c223b4370b3a

Note You need to log in before you can comment on or make changes to this bug.