wasmtime
wasmtime copied to clipboard
cranelift: Fix i128 iconst on AArch64 with egraphs
👋 Hey,
This is the first of the bugs that the fuzzer found in the new egraph work. On AArch64 iconst.i128 is not implemented. This PR implements it for values that fit in a u64, which is (I think) all the values that can be passed in by the frontend.
We've had discussions in the past about removing iconst.i128 (I can't find the link), and I think I've previously disagreed with this, but maybe we can restrict iconst to i64 and add a helper method to the frontend that does the right thing? so, for example for -1 we can lower it to iconst.i64 -1 + sextend, or iconst + iconst + iconcat for other values etc..
What do you guys think about that?
cc: @cfallin
It's interesting that this was hit only when egraph-based optimization is on -- I suspect because of the different way that constants are rematerialized...
In any case, I do think that disallowing iconst.i128 makes the most sense: the type is kind of a lie, because the instruction can't actually carry a 128-bit payload. Given that it's not fully general, producers need to do something else to emit an arbitrary 128-bit constant anyway, so we might as well remove the partial support and relieve the backends of some complexity.
Closing this since I'm working on removing iconst.i128 instead