Stream: git-wasmtime

Topic: wasmtime / issue #7203 riscv64: Improve codegen for `icmp`

Wasmtime GitHub notifications bot (Oct 10 2023 at 10:44):

github-actions[bot] commented on issue #7203:

Subscribe to Label Action

cc @cfallin, @fitzgen

<details>
This issue or pull request has been labeled: "cranelift", "isle"

Thus the following users have been cc'd because of the following labels:

• cfallin: isle
• fitzgen: isle

To subscribe or unsubscribe from this label, edit the <code>.github/subscribe-to-label.json</code> configuration file.

Learn more.
</details>

Wasmtime GitHub notifications bot (Oct 11 2023 at 05:59):

afonso360 commented on issue #7203:

Fuzzing has turned up the following testcase:

test interpret
test run
target riscv64gc

function %a() -> i8 {
block0:
    v9 = iconst.i8 128
    v11 = iconst.i8 0
    v17 = icmp uge v9, v11  ; v9 = 128, v11 = 0
    return v17
}

; run: %a() == 1

Wasmtime GitHub notifications bot (Oct 11 2023 at 06:16):

alexcrichton commented on issue #7203:

Do you have native RISC-V hardware that you're fuzzing on? Or are you running fuzzing through QEMU emulation? If the latter I think I'll try to throw some compute at this as well to help weed out more too.

Wasmtime GitHub notifications bot (Oct 11 2023 at 06:19):

afonso360 commented on issue #7203:

I've been running this natively, but honestly it's not much faster than running it on QEMU on my desktop so it might help. This example took like 8 hours to show up.

Wasmtime GitHub notifications bot (Oct 11 2023 at 06:19):

afonso360 edited a comment on issue #7203:

I've been running this natively, but honestly it's not much faster than running it on QEMU on my desktop, so it might help. This example took like 8 hours to show up.

Wasmtime GitHub notifications bot (Oct 11 2023 at 06:25):

afonso360 edited a comment on issue #7203:

I've been running this natively, but honestly it's not much faster than running it on QEMU on my desktop, so it might help. This example took like 8 hours to show up.

For comparison, I'm getting about 110 execs/s/core * 4 cores. (with -max_len=128)

Wasmtime GitHub notifications bot (Oct 11 2023 at 06:26):

afonso360 edited a comment on issue #7203:

I've been running this natively, but honestly it's not much faster than running it on QEMU on my desktop, so it might help. This example took like 8 hours to show up.

For comparison, Natively I'm getting about 110 execs/s/core * 4 cores. (with -max_len=128)

Wasmtime GitHub notifications bot (Oct 11 2023 at 06:26):

afonso360 edited a comment on issue #7203:

I've been running this natively, but honestly it's not much faster than running it on QEMU on my desktop, so it might help. This example took like 8 hours to show up.

For comparison, Natively I'm getting about 50 execs/s/core * 4 cores. (with -max_len=128)

Wasmtime GitHub notifications bot (Oct 11 2023 at 12:31):

afonso360 edited a comment on issue #7203:

I've been running this natively, but honestly it's not much faster than running it on QEMU on my desktop, so it might help. This example took like 8 hours to show up.

For comparison, Natively I'm getting about 50 execs/s/core * 4 cores. (with -max_len=128)

Edit: Also, if you're going to run the fuzzer, you might want to pull #7208 in, that triggered fairly quickly for me.

Wasmtime GitHub notifications bot (Oct 12 2023 at 06:19):

alexcrichton commented on issue #7203:

Ok so I tried a bit of fuzzing locally and it found a number of crashes but I was unfortunately unable to reproduce outside of the fuzzer to diagnose further. The test cases all reproduce on main however so I don't think they're related to this PR and at least one of them reproduced after I applied the addi4spn fix too. Reproduction has been difficult due to the fact that these tests are using libcalls I believe. One issue to fix was that clif-util test doesn't support libcalls in its test interpret mode, but while that was easy enough to fix I hit a panic where relocations failed to be appplied because I assume the libcall target was >4G away as an assertion in Reloc::RiscvCallPlt was tripped.

In any case I remove the iconst-is-always-extended logic and otherwise I'll likely be relying on the fuzzing your doing for this as I've not been too successful locally. It did run for a few hours though without finding other bugs which seems good!

Additionally I removed the rules that modify an immediate with +1 since that was the cause of the issue fuzzing found above. I'll need to look more at that later, but figure this may be good to get in without blocking on that.

Wasmtime GitHub notifications bot (Oct 12 2023 at 09:03):

afonso360 commented on issue #7203:

Wow, that's interesting, I had noticed the clif-util test can't run libcalls, but had forgotten about it since It doesn't cause issues for me that often. But I'm going to try to investigate the other ones.

In any case, I'm going to start fuzzing these latest changes.

Wasmtime GitHub notifications bot (Oct 12 2023 at 14:46):

alexcrichton commented on issue #7203:

crashes.tar.gz are the crashes that fuzzing found locally for me

Wasmtime GitHub notifications bot (Oct 12 2023 at 18:02):

afonso360 commented on issue #7203:

This one showed up today:

test interpret
test run
target riscv64gc

function %a() -> i8  {
block0:
    v11 = iconst.i8 196
    v17 = icmp ugt v11, v11
    return v17
}

; run: %a() == 0

Wasmtime GitHub notifications bot (Oct 14 2023 at 16:16):

alexcrichton commented on issue #7203:

Ooh nice catch, more signed-vs-unsigned handling that needed fixing (fuzzing is great!)

Wasmtime GitHub notifications bot (Oct 16 2023 at 11:53):

afonso360 commented on issue #7203:

This has been fuzzing continuously for the past 36 hours without any crashes :tada: I'm going to consider that good enough.

Last updated: Nov 22 2024 at 16:03 UTC