Stream: git-wasmtime

Topic: wasmtime / issue #5446 cranelift: Add a conditional branc...

Wasmtime GitHub notifications bot (Dec 16 2022 at 01:59):

elliottt commented on issue #5446:

I haven't reviewed the backend lowerings, but I looked at everything else, and this is looking pretty solid to me. I did spot a few small typos, but mostly I have questions about the verifier and the lexer.

The backend lowerings were all copied from each backend's current implementation of brnz, so it should hopefully be a quick review there :+1:

Wasmtime GitHub notifications bot (Dec 16 2022 at 18:14):

afonso360 commented on issue #5446:

:wave: Hey,

I'm not sure how ready or not this is, but I was playing around with adding fuzzer support for it (see this branch) and it found a regalloc issue. (Both on x86_64 and AArch64)

<details>
<summary>Test Case</summary>

test interpret
test run
target aarch64
target x86_64

function %a() -> i8 system_v {
block0:
    v1 = iconst.i8 35
    brif v1, block1(v1), block1(v1)  ; v1 = 35

block1(v0: i8):
    return v0
}

; run: %a() == 35

</details>

<details>
<summary>Result</summary>

ERROR cranelift_codegen::machinst::compile > Register allocation error for vcode
VCode {
  Entry block: 0
Block 0:
    (original IR block: block0)
    (successor: Block 1)
    (successor: Block 2)
    (instruction range: 0 .. 3)
  Inst 0: movl    $35, %v132l
  Inst 1: testb   %v132b, %v132b
  Inst 2: jnz     label1; j label2
Block 1:
    (successor: Block 3)
    (instruction range: 3 .. 4)
  Inst 3: jmp     label3
Block 2:
    (successor: Block 3)
    (instruction range: 4 .. 5)
  Inst 4: jmp     label3
Block 3:
    (original IR block: block1)
    (instruction range: 5 .. 6)
  Inst 5: ret
}

Error: Branch(Inst(2))
CLIF for error:
function u0:0() -> i8 system_v {
block0:
    v1 = iconst.i8 35
    brif v1, block1(v1), block1(v1)  ; v1 = 35

block1(v0: i8):
    return v0
}

thread 'worker #0' panicked at 'register allocation: Branch(Inst(2))', cranelift/codegen/src/machinst/compile.rs:67:14
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
 ERROR cranelift_filetests::concurrent      > FAIL: panicked in worker #0: register allocation: Branch(Inst(2))
FAIL ./lmao.clif: panicked in worker #0: register allocation: Branch(Inst(2))
1 tests
Error: 1 failure

</details>

Wasmtime GitHub notifications bot (Dec 16 2022 at 18:48):

elliottt commented on issue #5446:

wave Hey,

I'm not sure how ready or not this is, but I was playing around with adding fuzzer support for it (see this branch) and it found a regalloc issue. (Both on x86_64 and AArch64)

Interesting! I'm surprised to see this, as the lowering for brif should match brnz, but modifying the test to use brnz / jump doesn't show the same problem. Thanks for the test case!

Wasmtime GitHub notifications bot (Dec 16 2022 at 19:53):

elliottt commented on issue #5446:

@afonso360 that bug is fixed now -- I hadn't updated lower_branch_blockparam_args to account for the fact that brif doesn't store its block params in the varargs of the branch instruction itself. Thanks for finding this!

Wasmtime GitHub notifications bot (Dec 18 2022 at 11:56):

afonso360 commented on issue #5446:

Here's another one. This one only triggers with use_egraphs. It also doesn't require any opt_level which is weird to me, because I thought that we only enabled egraph optimizations at some opt_level?

<details>
<summary>Testcase</summary>

```
test compile
set use_egraphs=true
target aarch64
target s390x
target riscv64
target x86_64

function %a(i32, i8) -> i32 {
block0(v0: i32, v1: i8):
v2 = iconst.i32 0x00cd_cdcd
brif v1, block1(v0), block1(v2)

block1(v3: i32):
return v3
}
```
</details>

<details>
<summary>Error</summary>

thread 'worker #0' panicked at 'assertion failed: regs.is_valid()', cranelift/codegen/src/machinst/lower.rs:1321:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:584:5
   1: core::panicking::panic_fmt
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:142:14
   2: core::panicking::panic
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/core/src/panicking.rs:48:5
   3: cranelift_codegen::machinst::lower::Lower<I>::put_value_in_regs
             at ./codegen/src/machinst/lower.rs:1321:9
   4: cranelift_codegen::machinst::lower::Lower<I>::lower_branch_blockparam_args
             at ./codegen/src/machinst/lower.rs:964:28
   5: cranelift_codegen::machinst::lower::Lower<I>::lower_clif_branches
             at ./codegen/src/machinst/lower.rs:939:9
   6: cranelift_codegen::machinst::lower::Lower<I>::lower
             at ./codegen/src/machinst/lower.rs:1046:21
   7: cranelift_codegen::machinst::compile::compile
             at ./codegen/src/machinst/compile.rs:37:9
   8: cranelift_codegen::isa::aarch64::AArch64Backend::compile_vcode
             at ./codegen/src/isa/aarch64/mod.rs:63:9
   9: <cranelift_codegen::isa::aarch64::AArch64Backend as cranelift_codegen::isa::TargetIsa>::compile_function
             at ./codegen/src/isa/aarch64/mod.rs:73:40
  10: cranelift_codegen::context::Context::compile_stencil
             at ./codegen/src/context.rs:143:9
  11: cranelift_codegen::context::Context::compile
             at ./codegen/src/context.rs:213:23
  12: <cranelift_filetests::test_compile::TestCompile as cranelift_filetests::subtest::SubTest>::run
             at ./filetests/src/test_compile.rs:56:29
  13: cranelift_filetests::subtest::SubTest::run_target
             at ./filetests/src/subtest.rs:97:13
  14: cranelift_filetests::runone::run
             at ./filetests/src/runone.rs:101:9
  15: cranelift_filetests::concurrent::worker_thread::{{closure}}::{{closure}}
             at ./filetests/src/concurrent.rs:150:46
  16: std::panicking::try::do_call
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:492:40
  17: __rust_try
  18: std::panicking::try
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panicking.rs:456:19
  19: std::panic::catch_unwind
             at /rustc/897e37553bba8b42751c67658967889d11ecd120/library/std/src/panic.rs:137:14
  20: cranelift_filetests::concurrent::worker_thread::{{closure}}
             at ./filetests/src/concurrent.rs:150:30
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
 ERROR cranelift_filetests::concurrent > FAIL: panicked in worker #0: assertion failed: regs.is_valid()
FAIL ./lmao.clif: panicked in worker #0: assertion failed: regs.is_valid()
1 tests
Error: 1 failure

</details>

Wasmtime GitHub notifications bot (Dec 18 2022 at 22:56):

elliottt commented on issue #5446:

@afonso360 thanks for finding that one! I found it separately on https://github.com/bytecodealliance/wasmtime/pull/5464. I decided that it would be a good idea to rework all branching instructions to use BlockWithArgs, and that showed all the places that I was overlooking on this branch. Ultimately #5464 will allow us to have block arguments with br_table instructions as well, so I'll try to land that and then rebase this PR on top of it.

Wasmtime GitHub notifications bot (Dec 19 2022 at 16:33):

jameysharp commented on issue #5446:

It also doesn't require any opt_level which is weird to me, because I thought that we only enabled egraph optimizations at some opt_level?

That's an interesting question. Looking at cranelift/codegen/src/context.rs, I guess use_egraphs entirely ignores the optimization level. Also, it looks like there's no difference in Cranelift between the different values of OptLevel except for testing whether it's OptLevel::None or not. So use_egraphs effectively forces a non-None optimization level.

Wasmtime GitHub notifications bot (Dec 19 2022 at 17:28):

cfallin commented on issue #5446:

It also doesn't require any opt_level which is weird to me, because I thought that we only enabled egraph optimizations at some opt_level?

That's an interesting question. Looking at cranelift/codegen/src/context.rs, I guess use_egraphs entirely ignores the optimization level. Also, it looks like there's no difference in Cranelift between the different values of OptLevel except for testing whether it's OptLevel::None or not. So use_egraphs effectively forces a non-None optimization level.

Indeed, the intent of that behavior (it was this way with original egraphs and kept with the refactor) was to make the flag a top-level override; when we delete the non-egraphs path it makes sense to use opt_level again to choose whether to use the egraph pass.

Wasmtime GitHub notifications bot (Jan 18 2023 at 17:35):

github-actions[bot] commented on issue #5446:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "cranelift:area:aarch64", "cranelift:area:machinst", "cranelift:area:x64", "cranelift:meta", "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 <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Jan 18 2023 at 18:30):

elliottt commented on issue #5446:

@afonso360 I added support for fuzzing on this branch by considering brif as one candidate for instruction generation in the BlockTerminator::Br case. The diff ended up a bit smaller than your original branch now that the builder for brif doesn't need to construct a BlockCall directly :+1:

Last updated: Nov 22 2024 at 17:03 UTC