Stream: git-wasmtime

Topic: wasmtime / PR #8695 riscv64: Initial support for the ZiCo...

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2024 at 11:09):

afonso360 edited PR #8695.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2024 at 11:18):

afonso360 updated PR #8695.

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2024 at 12:50):

github-actions[bot] commented on PR #8695:

Subscribe to Label Action

cc @cfallin, @fitzgen

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

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

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

Learn more.
</details>

view this post on Zulip Wasmtime GitHub notifications bot (May 27 2024 at 13:22):

afonso360 updated PR #8695.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 15:01):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 15:01):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 15:01):

alexcrichton created PR review comment:

Should these two rules perhaps be gated by zicond being available?

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 15:01):

alexcrichton created PR review comment:

I remember running into this historically as well, you wouldn't happen to know off-hand if there's an issue for this or if you've bottomed it out before perhaps? This isn't the first time that it's been useful to pattern-match on (zero_reg) and generating the zero register directly I think also results in better codegen in situations as well (as it avoids a move-of-zero into a register)

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 15:01):

alexcrichton created PR review comment:

Should there perhaps be one more fully-general rule for has_zicond which performs the compare given arbitrary values in the condition and then does the two czero.* instructions? That might be more optimal than falling back to the full Select instruction which is pretty branch-y.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 17:44):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 17:44):

afonso360 created PR review comment:

Yeah, I just double checked and that's also what LLVM emits. I'll add that.

I'm pretty surprised that this is better than the optimal sequence of 2 mv's and a conditional branch over one of them. But hey, it is what it is.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 17:46):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 17:46):

afonso360 created PR review comment:

I've definitely run into it in the past. And IIRC it was something to do with regalloc, but I don't remember what exactly. I tried to search around the existing issues / PR's and I couldn't find a direct reference to any of it, but I'm fairly sure we've talked about it.

I'll create an issue and then we can have that centralized in a single place.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:33):

afonso360 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:33):

afonso360 created PR review comment:

I found it! https://github.com/bytecodealliance/wasmtime/pull/7162 It looks like there's a pretty big incompatibility with RA2.

I wonder if we could just check all of the register uses when collecting the operands, and using [Operand::fixed_nonallocatable()](https://docs.rs/regalloc2/latest/regalloc2/struct.Operand.html#method.fixed_nonallocatable) as suggested if we do find the zero register. Might be worth trying.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:33):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:33):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:42):

afonso360 edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:44):

cfallin submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 18:44):

cfallin created PR review comment:

Unfortunately yeah the issue is that we don't do rematerialization, and so we need to distinguish between "zero for this operand that can accept a special reg" and "zero in an arbitrary lowered-from-SSA reg" which fundamentally needs participation from the lowering logic; operand kinds can't patch the hole.

(Remat would be great to have! It's also a giant lift in complexity...)

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 19:17):

alexcrichton submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (May 28 2024 at 19:17):

alexcrichton created PR review comment:

Nothing like routinely forgetting PRs I myself made...

In any case thanks for digging that up! In the meantime I would definitely agree that the implemented solution in this PR, pattern matching iconst 0, is the way to go.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2024 at 12:09):

afonso360 updated PR #8695.

view this post on Zulip Wasmtime GitHub notifications bot (May 29 2024 at 12:24):

afonso360 commented on PR #8695:

It looks like one of the CI jobs was canceled part way through. I wonder if this is also related to the issues in #8701

view this post on Zulip Wasmtime GitHub notifications bot (May 30 2024 at 19:56):

alexcrichton merged PR #8695.

Last updated: Nov 22 2024 at 16:03 UTC