Cranelift: Fix union node bitpacking #7465

fitzgen · 2023-11-02T20:11:09Z

It turns out we have just been taking the newest rewrite's value for a eclass union and never actually comparing costs and taking the value with the minimum cost. Whoops!

Fixing this made some test expectations fail, which we resolved by tweaking the cost function to give materializing constants nonzero cost. This way we prefer -x to 0 - x.

We also made elaboration function break ties between values with the same cost with the value index. It prefers larger value indices, since the original value's index will be lower than all of its rewritten values' indices. This heuristically prefers rewritten values because we hope our rewrites are all improvements even when the cost function can't show that.

cfallin

LGTM, and thanks for finding this!

cfallin · 2023-11-03T15:59:34Z

Last merge-queue test run seemed to show a real assert failure during compilation:

thread '<unnamed>' panicked at 'assertion failed: x < (1 << bits)', cranelift/codegen/src/ir/dfg.rs:557:9

weird that it only reproduces on the MSRV run and for an integration test?

alexcrichton · 2023-11-03T16:03:35Z

I'm going to remove from the queue due to ^

fitzgen · 2023-11-03T16:04:38Z

It looks like we are creating a union of two values and the values' indices don't fit in the packed representation

fitzgen · 2023-11-03T16:16:45Z

Does anyone know how to run the tokio example, or at least build the wasms that it uses?

cargo build --target wasm32-wasi inside examples/tokio/wasm doesn't seem to actually produce any wasm files for me.

alexcrichton · 2023-11-03T16:20:36Z

cargo build --target wasm32-wasi --manifest-path examples/tokio/wasm/Cargo.toml
cargo run --example tokio --features wasmtime-wasi/tokio

fitzgen · 2023-11-03T18:14:56Z

Hm I can't reproduce the assertion failure for that example.

But I did find an off-by-one bug in the assertion. It allows 0x00ff_ffff to be encoded as-is but that conflicts with the sentinal we use to represent 0xffff_ffff. Tweaked to fix the assertion.

fitzgen · 2023-11-03T18:30:22Z

Okay I was able to repro with the 1.71.0 toolchain. Interestingly enough, it seemed to hang for a while before actually panicking, when the other toolchains ran to completion almost immediately.

fitzgen · 2023-11-03T19:14:51Z

Still going (it is generating a lot of output) but it looks like we've gone into some kind of loop where we are unioning values like x, x+1, x+2, .... Since this only shows up on 1.71.0, I'm thinking this might be a miscompile in rustc/llvm?

FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db0, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db1, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db2, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db3, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db4, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db5, y = 0xc29d09)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db6, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db7, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db8, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90db9, y = 0xc29d09)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dba, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dbb, y = 0xc29d1f)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dbc, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dbd, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dbe, y = 0xc29d09)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dbf, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dc0, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dc1, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dc2, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xe90dc3, y = 0xc29d09)

fitzgen · 2023-11-03T19:15:30Z

Yep, it just finished:

FITZGEN: packing union(tag = 11, ty = 79, x = 0xfffff9, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xfffffa, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xfffffb, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xfffffc, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xfffffd, y = 0xc29cf9)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xfffffe, y = 0xc29d09)
FITZGEN: packing union(tag = 11, ty = 79, x = 0xffffff, y = 0xc29cf9)
thread '<unnamed>' panicked at '16777215 does not fit into 24 bits (must be less than 16777215 to allow for a 0xffffffff sentinal)', cranelift/codegen/src/ir/dfg.rs:558:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

github-actions · 2023-11-04T01:03:43Z

Subscribe to Label Action

cc @cfallin, @fitzgen

This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

cfallin: isle
fitzgen: isle

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

fitzgen · 2023-11-06T17:34:00Z

I can't reproduce the test failure in CI. I'm also on linux x64 and using rust 1.73. Not sure what is up here.

afonso360 · 2023-11-07T09:35:55Z

Hey, I ran the fuzzer on RISC-V overnight on this branch to check if there is something wrong with the RISC-V backend. I don't know if there is yet, but I think I got a reproduction of the issue that was happening in CI earlier.

Testcase

;; Run test case

test interpret
test run
set opt_level=speed_and_size
target riscv64gc has_zca has_zcd

function %a(i8 sext, i16, i64 sext, i128 uext, i64, i128 uext, i64 uext, i64 uext, i64 uext, i128 uext, i32 sext, f64, i64 sext) -> i8 fast {
    ss0 = explicit_slot 29
    ss1 = explicit_slot 17
    ss2 = explicit_slot 49
    ss3 = explicit_slot 52
    ss4 = explicit_slot 17
    ss5 = explicit_slot 17

block0(v0: i8, v1: i16, v2: i64, v3: i128, v4: i64, v5: i128, v6: i64, v7: i64, v8: i64, v9: i128, v10: i32, v11: f64, v12: i64):
    v54 -> v1
    v37 = rotl v0, v10
    v38 = rotl v37, v10
    v39 = rotl v38, v10
    v40 = rotl v39, v10
    v41 = rotl v40, v10
    v42 = rotl v41, v10
    v43 = select_spectre_guard v1, v10, v10
    v44 = bxor_not v43, v43
    v55 -> v44
    v45 = bitrev v42
    v46 = sextend.i128 v45
    v47 = rotl v45, v44
    v48 = rotl v47, v44
    v49 = select v48, v48, v48
    v50 = rotl v49, v44
    v51 = rotl v50, v44
    v52 = rotl v51, v44
    v53 = rotl v52, v44
    v58 -> v53
    v56 = select_spectre_guard.i32 v54, v55, v55
    v57 = bxor_not v56, v56
    v59 = bitrev.i8 v58
    v61 = rotl v59, v57
    v62 = rotl v61, v57
    v63 = select v62, v62, v62
    v64 = rotl v63, v57
    v65 = rotl v64, v57
    v66 = rotl v65, v57
    v67 = rotl v66, v57
    v68 = rotr v67, v54
    v69 = rotl v68, v57
    v70 = rotl v69, v57
    v71 = rotl v70, v57
    v72 = rotl v71, v57
    v73 = rotl v72, v57
    return v73
}

; run: %a(17, 4474, 4803839602528529, -10005959247738946646150328352768, 19070975, 281564249487374563584566874706289408, 8440, 0, 0, 0, 0, 0.0, 0) == 34

Egraphs Result `main`

function %a(i8 sext, i16, i64 sext, i128 uext, i64, i128 uext, i64 uext, i64 uext, i64 uext, i128 uext, i32 sext, f64, i64 sext) -> i8 fast {
block0(v0: i8, v1: i16, v2: i64, v3: i128, v4: i64, v5: i128, v6: i64, v7: i64, v8: i64, v9: i128, v10: i32, v11: f64, v12: i64):
    v43 = select_spectre_guard v1, v10, v10
    v91 = iconst.i32 0xffff_ffff
    v56 = select_spectre_guard v1, v91, v91  ; v91 = 0xffff_ffff, v91 = 0xffff_ffff
    v76 = iadd v10, v10
    v79 = iadd v76, v10
    v82 = iadd v79, v10
    v85 = iadd v82, v10
    v88 = iadd v85, v10
    v89 = rotl v0, v88
    v45 = bitrev v89
    v215 = iconst.i32 2
    v217 = rotl v45, v215  ; v215 = 2
    v59 = bitrev v217
    v308 = uextend.i32 v1
    v310 = isub v308, v215  ; v215 = 2
    v311 = ineg v310
    v404 = iconst.i32 0xffff_fffb
    v406 = iadd v311, v404  ; v404 = 0xffff_fffb
    v408 = rotl v59, v406
    return v408
}

Egraphs Result `fix-egraph-union`

function %a(i8 sext, i16, i64 sext, i128 uext, i64, i128 uext, i64 uext, i64 uext, i64 uext, i128 uext, i32 sext, f64, i64 sext) -> i8 fast {
block0(v0: i8, v1: i16, v2: i64, v3: i128, v4: i64, v5: i128, v6: i64, v7: i64, v8: i64, v9: i128, v10: i32, v11: f64, v12: i64):
    v43 = select_spectre_guard v1, v10, v10
    v251 = iconst.i32 0xffff_ffff
    v56 = select_spectre_guard v1, v251, v251  ; v251 = 0xffff_ffff, v251 = 0xffff_ffff
    v76 = iadd v10, v10
    v79 = iadd v76, v10
    v93 = iadd v79, v10
    v130 = iadd v10, v93
    v176 = iadd v10, v130
    v177 = rotl v0, v176
    v45 = bitrev v177
    v679 = iconst.i32 10
    v681 = rotl v45, v679  ; v679 = 10
    v59 = bitrev v681
    v376 = iconst.i32 0xffff_fffd
    v1116 = uextend.i32 v1
    v1161 = isub v376, v1116  ; v376 = 0xffff_fffd
    v1936 = rotl v59, v1161
    return v1936
}

This testcase spends a huge amount of time in egraphs with this PR and it now fails where it didn't before. Although I'm not 100% sure this isn't a bug in one of the rules that wasn't discovered previously.

~~I'm going to look into this again later today~~

Edit: This ended up being a RISC-V bug (#7491), but I think it's still worth looking at the testcase, we now spend a bunch of time building values which I'm not sure is intended.

fitzgen · 2023-11-07T17:17:28Z

Thanks for digging into that @afonso360!

It turns out we have just been taking the newest rewrite's value for a eclass union and never actually comparing costs and taking the value with the minimum cost. Whoops! Fixing this made some test expectations fail, which we resolved by tweaking the cost function to give materializing constants nonzero cost. This way we prefer `-x` to `0 - x`. We also made elaboration function break ties between values with the same cost with the value index. It prefers larger value indices, since the original value's index will be lower than all of its rewritten values' indices. This heuristically prefers rewritten values because we hope our rewrites are all improvements even when the cost function can't show that. Co-Authored-By: Chris Fallin <chris@cfallin.org> Co-Authored-By: Trevor Elliott <telliott@fastly.com>

We generally want to clamp down and avoid runaway behavior here. But there also seems to be some sort of rustc/llvm bug on Rust 1.71 that is causing iteration to wild here. This commit avoids that bug.

fitzgen requested a review from a team as a code owner November 2, 2023 20:11

fitzgen requested review from abrown and removed request for a team November 2, 2023 20:11

cfallin approved these changes Nov 2, 2023

View reviewed changes

cfallin enabled auto-merge November 2, 2023 20:14

cfallin added this pull request to the merge queue Nov 2, 2023

cfallin mentioned this pull request Nov 2, 2023

Cranelift: Break op cost ties with expression depth in egraphs #7456

Merged

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 2, 2023

github-actions bot added the cranelift Issues related to the Cranelift code generator label Nov 2, 2023

cfallin added this pull request to the merge queue Nov 2, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 2, 2023

fitzgen added this pull request to the merge queue Nov 3, 2023

alexcrichton removed this pull request from the merge queue due to a manual request Nov 3, 2023

fitzgen enabled auto-merge November 3, 2023 21:58

fitzgen disabled auto-merge November 3, 2023 22:20

fitzgen force-pushed the fix-egraph-union branch from e7b8618 to 77ed10a Compare November 3, 2023 22:30

github-actions bot added the isle Related to the ISLE domain-specific language label Nov 4, 2023

fitzgen force-pushed the fix-egraph-union branch from 77ed10a to 3b54957 Compare November 6, 2023 18:07

fitzgen added this pull request to the merge queue Nov 6, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 6, 2023

fitzgen and others added 6 commits November 7, 2023 09:59

Add more information to assertion message

2e894b3

Fix off-by-one bug in assertion

ee4d933

Limit number of matches consumed from ISLE

71d8233

We generally want to clamp down and avoid runaway behavior here. But there also seems to be some sort of rustc/llvm bug on Rust 1.71 that is causing iteration to wild here. This commit avoids that bug.

Update test expectation

a1646b2

prtest:full

55e882d

fitzgen force-pushed the fix-egraph-union branch from ab918f6 to 55e882d Compare November 7, 2023 17:59

fitzgen added this pull request to the merge queue Nov 7, 2023

Merged via the queue into bytecodealliance:main with commit e39c6b7 Nov 7, 2023
40 checks passed

fitzgen deleted the fix-egraph-union branch November 7, 2023 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cranelift: Fix union node bitpacking #7465

Cranelift: Fix union node bitpacking #7465

fitzgen commented Nov 2, 2023

cfallin left a comment

cfallin commented Nov 3, 2023

alexcrichton commented Nov 3, 2023

fitzgen commented Nov 3, 2023

fitzgen commented Nov 3, 2023

alexcrichton commented Nov 3, 2023

fitzgen commented Nov 3, 2023 •

edited

fitzgen commented Nov 3, 2023

fitzgen commented Nov 3, 2023

fitzgen commented Nov 3, 2023

github-actions bot commented Nov 4, 2023

fitzgen commented Nov 6, 2023

afonso360 commented Nov 7, 2023 •

edited

fitzgen commented Nov 7, 2023

Cranelift: Fix union node bitpacking #7465

Cranelift: Fix union node bitpacking #7465

Conversation

fitzgen commented Nov 2, 2023

cfallin left a comment

Choose a reason for hiding this comment

cfallin commented Nov 3, 2023

alexcrichton commented Nov 3, 2023

fitzgen commented Nov 3, 2023

fitzgen commented Nov 3, 2023

alexcrichton commented Nov 3, 2023

fitzgen commented Nov 3, 2023 • edited

fitzgen commented Nov 3, 2023

fitzgen commented Nov 3, 2023

fitzgen commented Nov 3, 2023

github-actions bot commented Nov 4, 2023

Subscribe to Label Action

fitzgen commented Nov 6, 2023

afonso360 commented Nov 7, 2023 • edited

fitzgen commented Nov 7, 2023

fitzgen commented Nov 3, 2023 •

edited

afonso360 commented Nov 7, 2023 •

edited