riscv64: Delete SelectIf instruction#5888
Conversation
1dbba89 to
50a723d
Compare
This comment was marked as outdated.
This comment was marked as outdated.
50a723d to
497922f
Compare
elliottt
left a comment
There was a problem hiding this comment.
This is great! I had a question about perhaps reusing lower_bmask and reworking the SelectReg lowering, but they're not blockers in my mind.
| (c Reg (rv_srli b (imm12_const 63))) | ||
| (non_zero Reg (rv_andi c (imm12_const 1))) | ||
| ;; Based on the 0 or 1 value we now build a mask that is either 0 or -1 | ||
| (mask Reg (rv_addi non_zero (imm12_const -1))) |
There was a problem hiding this comment.
Is the reason we can't use lower_bmask here its ultimate use of gen_select_reg? Would it be bad to rework the lowering of SelectReg to take this approach instead?
There was a problem hiding this comment.
I hadn't even considered that this was a bmask operation! And yeah, I think this is a better lowering than what we currently have. However I check what LLVM produces for bmask and It's slightly better, so I'm going to rework bmask to use that and use that here.
There was a problem hiding this comment.
I'm much happier with the output of this now, Thanks!
I'm now re-reading your comment, and realizing there was a second part about SelectReg that I missed.
I have somewhat mixed feelings about making Inst::SelectReg do something like this, It would be quite a bit longer (~4 instructions I think?). But it would be really nice since we could move it entirely into ISLE. However I don't think that justifies it, it's a really common operation in the backend.
I'm willing to be persuaded otherwise though!
There was a problem hiding this comment.
My worry with recommending lower_bmask was that it used gen_select_reg internally, which will produce jumps to select the result. Since lower_bmask no longer relies on gen_select_reg, I don't see any reason to rework the SelectReg emit code :)
Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
elliottt
left a comment
There was a problem hiding this comment.
This looks great to me, thanks Afonso!
| ; li t1,42 | ||
| ; andi t4,a0,255 | ||
| ; andi t1,t1,255 |
There was a problem hiding this comment.
It would be nice if normalize_cmp_value didn't normalize constants whose value already fit in the target type, but there's no need to fix that on this PR.
* riscv64: Delete `SelectIf` instruction * riscv64: Fix typo in comment Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> * riscv64: Improve `bmask` codegen * riscv64: Use `lower_bmask` in `select_spectre_guard` * riscv64: Use `lower_bmask` to extend values in `select_spectre_guard` Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> --------- Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
* riscv64: Delete `SelectIf` instruction * riscv64: Fix typo in comment Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> * riscv64: Improve `bmask` codegen * riscv64: Use `lower_bmask` in `select_spectre_guard` * riscv64: Use `lower_bmask` to extend values in `select_spectre_guard` Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> --------- Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
👋 Hey,
This PR deletes the
Inst::SelectIfpseudo instruction from the RISC-V backend. This instruction was only used when lowering theselect_spectre_guardclif instruction in a specific case of the input being preceded by anicmp. That being said, the instruction doesn't really do anything different from the regular select.This PR removes the instruction and defaults to the regular
Inst::Select. We get pretty much exactly the same codegen, with the exception that an additionalandiis now added into the generated code. That doesn't seem to be to bad, and we can probably remove it with some additional effort if needed.