The default bug view has changed. See this FAQ.

IonMonkey crashes on box2d benchmark

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: azakai, Assigned: jandem)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
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)

Updated

5 years ago
Assignee: general → jdemooij
Status: NEW → ASSIGNED
(Assignee)

Comment 1

5 years ago
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.
Attachment #602339 - Flags: review?(dvander)
(Assignee)

Comment 2

5 years ago
(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!)
(Reporter)

Comment 3

5 years ago
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?
(Reporter)

Comment 4

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 732861
(Assignee)

Comment 5

5 years ago
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)
(Assignee)

Comment 6

5 years ago
(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.
(Assignee)

Comment 7

5 years ago
Review ping.
(Reporter)

Comment 8

5 years ago
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+
(Assignee)

Comment 10

5 years ago
Pushed with nit fixed.

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