Cranelift: `br_table.i128` not implemented on x86

[afonso360]

👋 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

• 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_64

cc @abrown @jlb6740

[bjorn3]

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]

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

[bjorn3]

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]

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?

[afonso360]

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.

[bjorn3]

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.

[cfallin]

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. 😆

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?

[bjorn3]

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.

[cfallin]

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.