wasmtime
wasmtime copied to clipboard
cranelift: Remove `iconst.i128`
👋 Hey,
This is a fix for the issue that was discovered in #5046 where iconst.i128 was causing issues and we decided to remove it.
I think most of the codegen differences are different register selection, except for the x64 tests where it de-duplicated some zeroing of registers.
Bugpoint also had to be updated to recognize the new pattern of uextend+iconst when minimizing these cases.
cc: @cfallin
Hmm, I think something is still not quite right with bugpoint, it produces the correct results, but it does a bunch of passes before getting there.
I found some issues while fuzzing with egraphs after applying this patch where rematerialization is still inserting an iconst.i128, I want to chase that down before merging this:
Here's the current test case:
test interpret
test run
set opt_level=speed_and_size
set use_egraphs=true
set enable_llvm_abi_extensions=true
target x86_64
function %a(i128) -> i128 system_v {
block0(v0: i128):
v1 = bxor v0, v0
return v1
}
; run: %a(1) == 0
I'm also worried that simple_preopt might do something similar and we don't have any tests that trigger it.
Ah, that's almost certainly this rewrite rule:
(rule (simplify (bxor ty x x))
(subsume (iconst ty (imm64 0))))
I think a (fits_in_64 ty) extractor in place of ty in the LHS should fix it -- and there are a few cases below (here for band) as well.
So, the bxor was a simple fix.
I don't think we ever trigger that issue with the band rules, because if I'm reading them correctly they depend on one of the sides being a iconst.i128 to trigger, and we no longer have those so I don't think those rules ever fire in this situation. (I also haven't seen the fuzzer complain about those yet!)
We had some simple_preopt optimizations building iconst.i128, notably band_imm 0 and bor_imm -1, I've decided to disable those for I128's.
They have the same issue that we have with iconst.i128 where their operand is 64 bits but its a 128 bit operation and some sort of implicit extension is required. They are also on their way out with #4721.
Otherwise the fuzzer has stopped complaining about inserting iconst.i128's and we should be able to merge this!