1132888 - Jit test failures with GVN off.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Douglas Crosher [:dougc]]

While exploring GVN, some jit test failures were noted with '--ion-gvn=off'.

./jit_test.py --show-failed-cmd --tbpl --args --ion-gvn=off js

fails on the following commands:

js --ion-gvn=off -f lib/prolog.js --js-cache .js-cache --ion-eager --ion-check-range-analysis --no-sse3 --no-threads --ion-offthread-compile=off -e "const platform='linux2'; const libdir='lib/'; const scriptdir='tests/asm.js/'" -f tests/asm.js/bug1126251.js

js --ion-gvn=off -f lib/prolog.js --js-cache .js-cache -e "const platform='linux2'; const libdir='lib/'; const scriptdir='tests/asm.js/'" -f tests/asm.js/testControlFlow.js

js --ion-gvn=off -f lib/prolog.js --js-cache .js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='lib/'; const scriptdir='tests/asm.js/'" -f tests/asm.js/testControlFlow.js

js --ion-gvn=off -f lib/prolog.js --js-cache .js-cache --baseline-eager -e "const platform='linux2'; const libdir='lib/'; const scriptdir='tests/asm.js/'" -f tests/asm.js/testControlFlow.js

js --ion-gvn=off -f lib/prolog.js --js-cache .js-cache --baseline-eager --no-fpu -e "const platform='linux2'; const libdir='lib/'; const scriptdir='tests/asm.js/'" -f tests/asm.js/testControlFlow.js

js --ion-gvn=off -f lib/prolog.js --js-cache .js-cache --no-baseline --no-ion -e "const platform='linux2'; const libdir='lib/'; const scriptdir='tests/asm.js/'" -f tests/asm.js/testControlFlow.js

The failure on bug1126251.js is avoided if --ion-check-range-analysis is removed which might be a clue.

Is the compiler expected to work with GVN disabled?

Might it be possible to move GVN and LICM after range analysis.

Comment 1

[Dan Gohman [:sunfish]]

Attachment: no-gvn.patch

The failure on bug 1126251.js is likely the same as bug 1130618.

Several of the others are caused by a codegen optimization to skip "trivial" blocks -- blocks containing just a goto (which occur because of split critical edges, etc.). If the function entry itself is a trivial block, the optimization gets confused because it assumes blocks in a sequence of trivial blocks are never explicitly entered. GVN optimizes code like this away. The attached patch fixes it, though it's a little rough.

Comment 2

[Douglas Crosher [:dougc]]

(In reply to Dan Gohman [:sunfish] from comment #1)
> Created attachment 8564274 [details] [diff] [review]
> no-gvn.patch

Thank you, the patch fixes the jit-test failures, except test bug1126251.js and I'll follow bug 1130618.

Fwiw moving GVN and LICM after range analysis seems to work with this patch and the workaround in bug 1133203. The large asm.js demos I use for testing still run fine.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2caa55ff11f03353ba4a50bd55d000d9fdf65461

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

New results:

    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/basic/'" -f js/src/jit-test/tests/basic/bug1296249.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/dce-with-rinstructions.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/dce-with-rinstructions.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/dce-with-rinstructions.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-arrays.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-arrays.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-arrays.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-lambdas.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-lambdas.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-lambdas.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-pgo=on -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-objects.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager --ion-pgo=on -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-objects.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-pgo=on --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-objects.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --no-unboxed-objects --ion-pgo=on -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-objects.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --no-unboxed-objects --ion-pgo=on --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-objects.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager --no-unboxed-objects --ion-pgo=on -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-objects.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-typed-array.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-typed-array.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/ion/'" -f js/src/jit-test/tests/ion/recover-typed-array.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --ion-eager --ion-offthread-compile=off -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/SIMD/'" -f js/src/jit-test/tests/SIMD/recover.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/SIMD/'" -f js/src/jit-test/tests/SIMD/recover.js
    js --ion-gvn=off -f js/src/jit-test/lib/prologue.js --js-cache js/src/jit-test/.js-cache --baseline-eager -e "const platform='linux2'; const libdir='js/src/jit-test/lib/'; const scriptdir='js/src/jit-test/tests/SIMD/'" -f js/src/jit-test/tests/SIMD/recover.js

Mostly tests with recover instructions, with the following assertion:
Assertion failure: input()->isRecoveredOnBailout() == mustBeRecovered_ (assertRecoveredOnBailout failed during compilation), at js/src/jit/Recover.cpp:1564

Redirecting to Nicolas, because I am not sure it's safe to just add an `if (!JitOptions.disableGvn)`.

Comment 5

[Nicolas B. Pierron [:nbp]]

I was tracking some of them last week.

Apparently the issue comes from the fact that we eagerly remove NOPs, even if they have a resume point, which is the case of all the Nop instructions added by AssertRecoveredOnBailout.  So, as we have no check to ensure that AssertRecoveredOnBailout infra works, we basically disabled our test suite.

Then, branch pruning basically did not removed the branches as expected, when the heuristics changed back in June.  Thus causing these failures.

I have some-what of a patch, fixing gvn, and modifying the test cases to heat-up a bit more for branch pruning to kick-in.

Comment 6

[Nicolas B. Pierron [:nbp]]

Attachment: part 1 - Re-add recover instruction for recovering typed array allocations.

This fix an issue raised in Bug 1276955, which is that the recover
instruction code path got removed after disabling the code for
assertRecoveredOnBailout.

Comment 7

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Prevent spewers from consuming almost all the ballast space.

Not related, but let's fix it as it caused me trouble while debugging.

Comment 8

[Nicolas B. Pierron [:nbp]]

Attachment: part 3 - Fix assertRecoveredOnBailout optimization assertion.

Re-enable the test cases, by bumping the number of iteration needed for
branch pruning to remove some unused branches.

Fix some of the expectations, in case of object with a large number of
dynamic slots.

Comment 9

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Test that we do not disable assertRecoveredOnBailout assertion.

This is a meta test case to ensure that this does not happen again.
This check that assertRecoveredOnBailout will fail as expected.

Comment 10

[Hannes Verschore [:h4writer]]

Comment on attachment 8804720 [details] [diff] [review]
part 2 - Prevent spewers from consuming almost all the ballast space.

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

::: js/src/jit/JitSpewer.cpp
@@ +306,5 @@
> +    // Do not use ensureBallast here, as this function is used for debugging,
> +    // and as such we should not attempt to report OOM failures when we are
> +    // debugging.
> +    if (!graph_->alloc().lifoAlloc()->ensureUnusedApproximate(TempAllocator::BallastSize))
> +        MOZ_CRASH("[OOM] Buy more RAM!");

Shouldn't this use AutoEnterOOMUnsafeRegion ?
And can you fix the message "Could not ensure enough ballast space after spewing graph information."

Comment 11

[Hannes Verschore [:h4writer]]

Comment on attachment 8804722 [details] [diff] [review]
part 3 - Fix assertRecoveredOnBailout optimization assertion.

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

Those testcases are really fragile sadly. Having specific tests per pass would really help here.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8804724 [details] [diff] [review]
part 4 - Test that we do not disable assertRecoveredOnBailout assertion.

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

I don't really understand the need for the "-11" exitstatus. Can't we just return early without doing anything?

Comment 13

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #12)
> Comment on attachment 8804724 [details] [diff] [review]
> part 4 - Test that we do not disable assertRecoveredOnBailout assertion.
> 
> Review of attachment 8804724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I don't really understand the need for the "-11" exitstatus. Can't we just
> return early without doing anything?

The SEGV is not catcheable, the "exitstatus: -11" is used to ensure that the jit-test harness consider this SEGV, from assertRecoveredOnBailout (Recover.cpp) as the expected value.

We have to replicate this exit code, if the jit are disabled, otherwise we would never enter assertRecoveredOnBailout, and inIon() will return a string as an error code.

This test case is here to ensure that we no longer disable assertRecoveredOnBailout by accident.

Comment 14

[Hannes Verschore [:h4writer]]

Comment on attachment 8804724 [details] [diff] [review]
part 4 - Test that we do not disable assertRecoveredOnBailout assertion.

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

::: js/src/jit-test/tests/self-test/assertRecoveredOnBailout-1.js
@@ +1,1 @@
> +// |jit-test| exitstatus: -11

Instead of exitstatus, we should have "crash" here as the expected result.
Can you add that instead of relying on -11 that might be platform specific?

@@ +1,5 @@
> +// |jit-test| exitstatus: -11
> +
> +var opts = getJitCompilerOptions();
> +if (!opts['ion.enable'] || !opts['baseline.enable'])
> +  quit(-11);

We should add a helper function "crash('foo');"

Comment 15

[Nicolas B. Pierron [:nbp]]

Attachment: part 2 - Prevent spewers from consuming almost all the ballast space.

Delta:
 - Apply nit.
 - Add MIRGraph header.

Comment 16

[Nicolas B. Pierron [:nbp]]

Attachment: part 4 - Test that we do not disable assertRecoveredOnBailout assertion.

Delta:
 - Use the existing crash builtin of the shell.
 - Add "|jit-test| crash" support, with Windows error code (reported on try)
 - Add --ion-check-range-analysis flag to getJitCompilerOptions (as
     MAssertRecoveredOnBailout is not produced if it is enabled)

Comment 17

[Hannes Verschore [:h4writer]]

Comment on attachment 8805612 [details] [diff] [review]
part 2 - Prevent spewers from consuming almost all the ballast space.

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

Thanks!

Comment 18

[Hannes Verschore [:h4writer]]

Comment on attachment 8805617 [details] [diff] [review]
part 4 - Test that we do not disable assertRecoveredOnBailout assertion.

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

Nice! I assume you did a try run to be sure?

Comment 19

[Hannes Verschore [:h4writer]]

(In reply to Hannes Verschore [:h4writer] from comment #18)
> Comment on attachment 8805617 [details] [diff] [review]
> part 4 - Test that we do not disable assertRecoveredOnBailout assertion.
> 
> Review of attachment 8805617 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! I assume you did a try run to be sure?

Reading your comment that is a yes. Overlooked that. Thanks!

Comment 20

[Nicolas B. Pierron [:nbp]]

Yes, I did push to try, and but at the time I had submitted the ASan and ARM64 where not yet red.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=221b9349be3f5b7fbb1bcba6b91bbbcb59542b0a

I made a few changes, to fix this minor issues and double-checking with try again.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4b1a95a7f39
part 1 - Re-add recover instruction for recovering typed array allocations. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e2739dedc3a
part 2 - Prevent spewers from consuming almost all the ballast space. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/f596578a143e
part 3 - Fix assertRecoveredOnBailout optimization assertion. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd3ff163021b
part 4 - Test that we do not disable assertRecoveredOnBailout assertion. r=h4writer

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to bafck this out for Spidermonkey failures like https://treeherder.mozilla.org/logviewer.html#?job_id=38666405&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c43bca082b7

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #22)
> I had to bafck this out for Spidermonkey failures like
> https://treeherder.mozilla.org/logviewer.html#?job_id=38666405&repo=mozilla-
> inbound
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9c43bca082b7

It hit twice out of 9 total runs, if that helps you in your attempts to reproduce.

Comment 24

[Nicolas B. Pierron [:nbp]]

I tried "rr record -h" on an optimized build of the JS shell, without the root-analysis.
I did not managed to get this assertion while running a JS shell compiled by clang 3.7 (~2000 runs), but I did managed to get this assertion while running a JS shell compiled by gcc 4.8 (after ~275 runs).

I will investigate.

Comment 25

[Nicolas B. Pierron [:nbp]]

Ok, the problem faced with this intermittent is that we are compiling "rceil_number" later than expected, which gives some times to monitor the return type of "Math.ceil" as potentially being a double.

The reason this can now be a double is that we only measured in Baseline, and to accommodate from the branch pruning modifications, I added some extra cycle in the negative.  As other test failed when using negative numbers, I had to wrap around the values with Math.abs.  "rceil_number" does the ceiling of (-i -0.12…) which can be rounded to -0 when i is 0.

I've changed the wrap-around to add a positive immediate value to avoid the i == 0 case.

If the following try-push works, I will squash the simple fixup patch and re-push the changes to inbound.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=26a51f90f990eb7dbc50116f92816f8bb9d6f134

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/968aa5ce9ed4
part 1 - Re-add recover instruction for recovering typed array allocations. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ae0b40fd73
part 2 - Prevent spewers from consuming almost all the ballast space. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c1b90a1926d
part 3 - Fix assertRecoveredOnBailout optimization assertion. r=h4writer
https://hg.mozilla.org/integration/mozilla-inbound/rev/8daddd6054f2
part 4 - Test that we do not disable assertRecoveredOnBailout assertion. r=h4writer

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/968aa5ce9ed4
https://hg.mozilla.org/mozilla-central/rev/40ae0b40fd73
https://hg.mozilla.org/mozilla-central/rev/5c1b90a1926d
https://hg.mozilla.org/mozilla-central/rev/8daddd6054f2