afonso360 labeled issue #4498:
:wave: Hey,
Fuzzgen found this when I added i128 support.
.clif
Test Casefunction %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
- Run
cargo run -- compile -D --set enable_llvm_abi_extensions --target x86_64 .\the-above.clif
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_64cc @abrown @jlb6740
afonso360 opened issue #4498:
:wave: Hey,
Fuzzgen found this when I added i128 support.
.clif
Test Casefunction %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
- Run
cargo run -- compile -D --set enable_llvm_abi_extensions --target x86_64 .\the-above.clif
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_64cc @abrown @jlb6740
afonso360 labeled issue #4498:
:wave: Hey,
Fuzzgen found this when I added i128 support.
.clif
Test Casefunction %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
- Run
cargo run -- compile -D --set enable_llvm_abi_extensions --target x86_64 .\the-above.clif
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_64cc @abrown @jlb6740
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
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.
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 insertingbr_tables
https://github.com/bytecodealliance/wasmtime/blob/6e05b646a32fe00c390cc2322b1d7d3571526a9a/cranelift/frontend/src/switch.rs#L10-L41
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 insertingbr_tables
https://github.com/bytecodealliance/wasmtime/blob/6e05b646a32fe00c390cc2322b1d7d3571526a9a/cranelift/frontend/src/switch.rs#L10-L41
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.
jameysharp commented on issue #4498:
I guess
br_table
on ani128
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 supporti128
(ori64
either?) and avoid trying to generate such cases.@cfallin, what's your take?
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 targetsbr_table
, that way we don't get the hole in the api.
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 MIRSwitchInt
terminator usingcranelift_frontend::Switch
.
cfallin commented on issue #4498:
I guess
br_table
on ani128
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 supporti128
(ori64
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
takei32
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?
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 usingbr_table.i8
, but it would need to be forbr_table.i32
.
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.
jameysharp closed issue #4498:
:wave: Hey,
Fuzzgen found this when I added i128 support.
.clif
Test Casefunction %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
- Run
cargo run -- compile -D --set enable_llvm_abi_extensions --target x86_64 .\the-above.clif
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_64cc @abrown @jlb6740
Last updated: Dec 23 2024 at 13:07 UTC