Stream: git-wasmtime

Topic: wasmtime / issue #4498 Cranelift: `br_table.i128` not imp...

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 16:13):

afonso360 labeled issue #4498:

:wave: Hey,

Fuzzgen found this when I added i128 support.

.clif Test Case

function %br(f32, i128) -> i32 system_v {
    jt0 = jump_table [block1, block1, block1]

block0(v0: f32, v1: i128):
    br_table v1, block1, jt0
block1:
    v2 = iconst.i32 0
    return v2
}

Steps to Reproduce

Expected Results

The code to compile

Actual Results

     Running `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif`
thread 'main' panicked at 'BrTable unimplemented for I128', cranelift\codegen\src\isa\x64\lower.rs:3248:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif` (exit code: 101)

Versions and Environment

Cranelift version or commit: main
Operating system: Windows
Architecture: x86_64

cc @abrown @jlb6740

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 16:13):

afonso360 opened issue #4498:

:wave: Hey,

Fuzzgen found this when I added i128 support.

.clif Test Case

function %br(f32, i128) -> i32 system_v {
    jt0 = jump_table [block1, block1, block1]

block0(v0: f32, v1: i128):
    br_table v1, block1, jt0
block1:
    v2 = iconst.i32 0
    return v2
}

Steps to Reproduce

Expected Results

The code to compile

Actual Results

     Running `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif`
thread 'main' panicked at 'BrTable unimplemented for I128', cranelift\codegen\src\isa\x64\lower.rs:3248:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif` (exit code: 101)

Versions and Environment

Cranelift version or commit: main
Operating system: Windows
Architecture: x86_64

cc @abrown @jlb6740

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 16:13):

afonso360 labeled issue #4498:

:wave: Hey,

Fuzzgen found this when I added i128 support.

.clif Test Case

function %br(f32, i128) -> i32 system_v {
    jt0 = jump_table [block1, block1, block1]

block0(v0: f32, v1: i128):
    br_table v1, block1, jt0
block1:
    v2 = iconst.i32 0
    return v2
}

Steps to Reproduce

Expected Results

The code to compile

Actual Results

     Running `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif`
thread 'main' panicked at 'BrTable unimplemented for I128', cranelift\codegen\src\isa\x64\lower.rs:3248:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif` (exit code: 101)

Versions and Environment

Cranelift version or commit: main
Operating system: Windows
Architecture: x86_64

cc @abrown @jlb6740

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 17:54):

bjorn3 commented on issue #4498:

cranelift_frontend::Switch has a workaround for this in place already: https://github.com/bytecodealliance/wasmtime/blob/6e05b646a32fe00c390cc2322b1d7d3571526a9a/cranelift/frontend/src/switch.rs#L267-L280

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:18):

afonso360 commented on issue #4498:

That just reminded me that we had a similar issue previously #3100.

I tried to add a panic to that branch and it isn't being triggered, ill investigate further.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:22):

afonso360 edited a comment on issue #4498:

That just reminded me that we had a similar issue previously #3100.

I tried to add a panic to that branch and it isn't being triggered, ill investigate further.

Edit: I suspect that patch only applies to the Switch interface of the frontend, and not when building jump tables manually and then inserting br_tables
https://github.com/bytecodealliance/wasmtime/blob/6e05b646a32fe00c390cc2322b1d7d3571526a9a/cranelift/frontend/src/switch.rs#L10-L41

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:23):

afonso360 edited a comment on issue #4498:

That just reminded me that we had a similar issue previously #3100.

I tried to add a panic to that branch and it isn't being triggered, ill investigate further.

Edit: I suspect that workaround only applies to the Switch interface of the frontend, and not when building jump tables manually and then inserting br_tables
https://github.com/bytecodealliance/wasmtime/blob/6e05b646a32fe00c390cc2322b1d7d3571526a9a/cranelift/frontend/src/switch.rs#L10-L41

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:31):

bjorn3 commented on issue #4498:

Edit: I suspect that workaround only applies to the Switch interface of the frontend, and not when building jump tables manually and then inserting br_tables

Indeed. Should have been clearer.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:38):

jameysharp commented on issue #4498:

I guess br_table on an i128 is useless, right? It's going to be a while before anyone has enough memory for a table that big. :laughing:

For the purposes of cranelift-fuzzgen it'd be nice to have all operations support all types, but I imagine implementing this in each target backend requires extra code that isn't going to be exercised any other way. So I'm thinking it'd be better to just declare that br_table doesn't support i128 (or i64 either?) and avoid trying to generate such cases.

@cfallin, what's your take?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:45):

afonso360 commented on issue #4498:

Yeah, we don't even support tables larger than u32, so...

For the purposes of cranelift-fuzzgen it'd be nice to have all operations support all types

We special case emissions of br_table's in fuzzgen, so we can disable it quite easly.

So I'm thinking it'd be better to just declare that br_table doesn't support i128 (or i64 either?)

I think this is probably a good idea. Or we can do what we do in the Switch interface (patch it down to a i32) in some place that also targets br_table, that way we don't get the hole in the api.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 18:45):

bjorn3 commented on issue #4498:

I guess br_table on an i128 is useless, right? It's going to be a while before anyone has enough memory for a table that big. laughing

Rustc allows matching on 128bit integers and enums. That is why cranelift_frontend::Switch has a workaround for this. I'm lowering the MIR SwitchInt terminator using cranelift_frontend::Switch.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 19:03):

cfallin commented on issue #4498:

I guess br_table on an i128 is useless, right? It's going to be a while before anyone has enough memory for a table that big. :laughing:

For the purposes of cranelift-fuzzgen it'd be nice to have all operations support all types, but I imagine implementing this in each target backend requires extra code that isn't going to be exercised any other way. So I'm thinking it'd be better to just declare that br_table doesn't support i128 (or i64 either?) and avoid trying to generate such cases.

@cfallin, what's your take?

In general I'm in favor of more completeness, or at least symmetry -- the more our "supported instruction/operand space" looks like a high-dimensional rectangular prism, rather than a weird shape with lots of divots, the easier for our users. But I think that impulse should also be tempered by practicalities so we don't have blowup in the backends' pattern-matching, as that can lead to issues (copy/paste typos, difficulty of maintenance) as well.

I might actually propose a slightly different approach: could br_table take i32 only? My path to thinking that is that being polymorphic except for one unsupported type (i128) is actually leading users into a false sense of flexibility. And supporting narrow values has potential pitfalls too (I believe it works correctly now but rare cases are a risk both for new backends and during ongoing refactoring/improvements). A consistent mental model that supports this view (IMHO) is to think of the selector index as morally equivalent to a jump address, kind of. Addresses have a fixed width (64 bits on all our current platforms); so why not br_table indices (hereby declared to be 32 bits), as well?

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 19:11):

bjorn3 commented on issue #4498:

For br_table.i8 I think that would lead to a pessimization. For example if there are 256 entries, it wouldn't need to check if the input value is larger than 255 to jump to the default branch when using br_table.i8, but it would need to be for br_table.i32.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 21 2022 at 19:53):

cfallin commented on issue #4498:

That's true, but we don't currently do such an optimization; and in the future, we could in a more general way by doing an integer range analysis. Such an analysis would have a rule that a uextend from an i8 gives a result in range 0..256. I suspect that's a more general approach than a special case of range analysis based on types.

view this post on Zulip Wasmtime GitHub notifications bot (Jul 22 2022 at 23:32):

jameysharp closed issue #4498:

:wave: Hey,

Fuzzgen found this when I added i128 support.

.clif Test Case

function %br(f32, i128) -> i32 system_v {
    jt0 = jump_table [block1, block1, block1]

block0(v0: f32, v1: i128):
    br_table v1, block1, jt0
block1:
    v2 = iconst.i32 0
    return v2
}

Steps to Reproduce

Expected Results

The code to compile

Actual Results

     Running `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif`
thread 'main' panicked at 'BrTable unimplemented for I128', cranelift\codegen\src\isa\x64\lower.rs:3248:40
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `C:\Users\Afonso\CLionProjects\wasmtime\target\debug\clif-util.exe compile -D --set enable_llvm_abi_extensions --target x86_64 .\test.clif` (exit code: 101)

Versions and Environment

Cranelift version or commit: main
Operating system: Windows
Architecture: x86_64

cc @abrown @jlb6740

Last updated: Nov 22 2024 at 17:03 UTC