cranelift: Remove iconst.i128

[afonso360]

👋 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]

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.

[cfallin]

Thanks! This will be a nice simplification.

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

[cfallin]

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.

[afonso360]

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!

[cfallin]

Thanks for the updates! The non-orthogonality causing us to not do some rewrites is kind of unfortunate (and this is the cost we were worried about originally) but IMHO it's not too major, and the benefits (clarity in what will actually fit) outweigh it.