wasmtime icon indicating copy to clipboard operation
wasmtime copied to clipboard

cranelift: Remove `iconst.i128`

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

👋 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

afonso360 avatar Oct 19 '22 14:10 afonso360

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.

afonso360 avatar Oct 19 '22 14:10 afonso360

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.

afonso360 avatar Oct 19 '22 18:10 afonso360

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.

cfallin avatar Oct 19 '22 18:10 cfallin

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!

afonso360 avatar Oct 22 '22 16:10 afonso360