1573792 - Cranelift: fold redundant jumps causes new test failures

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: a.clif

I've found new test failures when compiling with Cranelift (jit_tests.py --args=--wasm-compiler=cranelift ./dist/bin/js wasm), and bisection lead to this Cranelift patch:

https://github.com/CraneStation/cranelift/commit/57fa45c8520110ee7e9b216538f1990376aed0e9

New test failures caused by this patch:

wasm/bench/wasm_box2d.js
wasm/regress/baseline-stack-height.js
wasm/spec/loop.wast.js

Sean, can you have a look, please? I've extracted the CLIF from the second one for you, and attached it to this bug. (It fails with standalone Cranelift, when running cargo run compile -d /tmp/a.clif.)

Comment 1

[Julian Seward [:jseward]]

After updating to CL tip this morning, I see failures like this on the embenchen test cases I am working with:

- inst679: uses value arg from non-dominating ebb7

- inst662: v233 is defined by ebb10 which is not in the layout

Might they be caused by this bug?

Comment 2

[Sean Stangl [:sstangl]]

Yeah, that's caused by this bug. Here's a reduced test-case that makes the error super obvious:

set opt_level=best

target x86_64

function u0:0() {

ebb0:
    v1 = iconst.i32 0
    jump ebb1(v1)

ebb1(v2: i32):
    jump ebb2

ebb2:
    brnz v2, ebb3
    jump ebb3

ebb3:
    return
}

Comment 3

[Sean Stangl [:sstangl]]

Filed a temporary workaround pull request: https://github.com/CraneStation/cranelift/pull/914

Comment 4

[Nicolas B. Pierron [:nbp]]

I guess one solution might be to alias v2 to v1 in ebb0. However, I am not sure how this would interact with diversions.

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

This was fixed by one of the recent Cranelift bumps, which included a Github commit reverting this change; also Sean made a simpler pass that works fine, iirc.