Stream: git-wasmtime

Topic: wasmtime / issue #5075 cranelift: Remove `iconst.i128`

Wasmtime GitHub notifications bot (Oct 19 2022 at 14:42):

afonso360 commented on issue #5075:

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.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:12):

afonso360 commented on issue #5075:

I found some issues 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

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:13):

afonso360 edited a comment on issue #5075:

I found some issues 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.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:13):

afonso360 edited a comment on issue #5075:

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.

Wasmtime GitHub notifications bot (Oct 19 2022 at 18:22):

cfallin commented on issue #5075:

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.

Wasmtime GitHub notifications bot (Oct 22 2022 at 16:05):

afonso360 commented on issue #5075:

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!

Last updated: Nov 22 2024 at 16:03 UTC