wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

cranelift: Fix i128 iconst on AArch64 with egraphs

Open afonso360 opened this issue 3 years ago • 1 comments
trafficstars

👋 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

afonso360 avatar Oct 12 '22 11:10 afonso360

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.

cfallin avatar Oct 12 '22 18:10 cfallin

Closing this since I'm working on removing iconst.i128 instead

afonso360 avatar Oct 17 '22 21:10 afonso360