afonso360 edited PR #8695.
afonso360 updated PR #8695.
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:
- 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>
afonso360 updated PR #8695.
alexcrichton submitted PR review.
alexcrichton submitted PR review.
alexcrichton created PR review comment:
Should these two rules perhaps be gated by
zicond
being available?
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)
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 twoczero.*
instructions? That might be more optimal than falling back to the fullSelect
instruction which is pretty branch-y.
afonso360 submitted PR review.
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.
afonso360 submitted PR review.
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.
afonso360 submitted PR review.
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.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
cfallin submitted PR review.
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...)
alexcrichton submitted PR review.
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.
afonso360 updated PR #8695.
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
alexcrichton merged PR #8695.
Last updated: Dec 23 2024 at 12:05 UTC