riscv64: Initial support for the ZiCond Extension #8695
Conversation
afonso360
added
the
cranelift:area:riscv64
afonso360
changed the title
github-actions
bot
added
cranelift
Subscribe to Label Action
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 |
Additionally add some checks to prevent a stack overflow in the zero swap rules
| ;; Manually matching (iconst 0) here is a bit of a hack. We can't do that as part | ||
| ;; of the iconst rule because that runs into regalloc issues. gen_select_xreg | ||
| ;; has some optimizations based on the use of the zero register so we have to | ||
| ;; manually match it here. |
There was a problem hiding this 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)
There was a problem hiding this 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.
There was a problem hiding this comment.
I found it! #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() instead of the normal reg_use if we do find the zero register. Might be worth trying.
Edit: Re-reading that thread again, it looks like this still wouldn't work
There was a problem hiding this 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...)
There was a problem hiding this 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.
This commit is inspired by discussion on bytecodealliance#8695 which made me remember the discussion around bytecodealliance#7162 historically. In lieu of a deeper fix for the issue of "why can't `iconst 0` use `(zero_reg)`" it's still possible to add special-cases to rules throughout the backend so this commit does that for generating zero-value floats.
|
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 |
* riscv64: Special-case `f32const 0` and `f64const 0` This commit is inspired by discussion on #8695 which made me remember the discussion around #7162 historically. In lieu of a deeper fix for the issue of "why can't `iconst 0` use `(zero_reg)`" it's still possible to add special-cases to rules throughout the backend so this commit does that for generating zero-value floats. * Fix tests * Run all tests on CI prtest:full
👋 Hey,
This PR adds support for the ZiCond: Integer Conditional Operations (Chapter 12 of the Unprivileged RISC-V ISA Manual).
This extension adds two instructions that either zero a register, or move one of the arguments, depending on a condition register.
czero.eqz rd, rs1, rs2- Moves zero to a register rd, if the condition rs2 is equal to zero, otherwise moves rs1 to rd.czero.nez rd, rs1, rs2- Moves zero to a register rd, if the condition rs2 is nonzero, otherwise moves rs1 to rdIt doesn't include a full conditional move of two registers, but that is implemented by combining both of the instructions above.
The ISA manual also suggests some interesting instruction sequences for conditional add/sub/and/or/etc... that only contain two instructions.
This PR only implements the base cases. The direct lowerings of the instructions, as well as the full conditional move of two registers. I'm planning on implementing the other sequences at a later date.