Closed Bug 732120 Opened 12 years ago Closed 12 years ago

IonMonkey crashes on box2d benchmark

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: azakai, Assigned: jandem)

References

Details

Attachments

(1 file)

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.
Assignee: general → jdemooij
Status: NEW → ASSIGNED
Attached patch rm ReorderBlocksSplinter Review
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.
Attachment #602339 - Flags: review?(dvander)
(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!)
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?
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?
Blocks: 732861
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.
Attachment #602339 - Flags: review?(dvander) → review?(sstangl)
(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.
Review ping.
Ping from me as well. This bug is preventing testing of IonMonkey on Emscripten-generated code.
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.
Attachment #602339 - Flags: review?(sstangl) → review+
Pushed with nit fixed.

https://hg.mozilla.org/projects/ionmonkey/rev/c223b4370b3a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.