[PATCH] D116215: [RISCV] Refactor immediate comparison instructions patterns

[Chenbing.Zheng via Phabricator via llvm-commits]

Chenbing.Zheng marked 3 inline comments as done.
Chenbing.Zheng added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:3726
+
+multiclass VPatCompareSpecial<string intrinsic, string inst> {
+  foreach vti = AllIntegerVectors in {
----------------
craig.topper wrote:
> Special -> "UnsignedZero" maybe?
Thanks,  it is a batter name.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:3726
+
+multiclass VPatCompareSpecil<string intrinsic, string inst> {
+  foreach vti = AllIntegerVectors in {
----------------
jrtc27 wrote:
> "Specil"? Assuming that's a typo for "Special" that's hardly a descriptive name
done


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:4526
+defm : VPatCompare_VI<"int_riscv_vmslt", "PseudoVMSLE">;
+defm : VPatCompare_VI<"int_riscv_vmsltu", "PseudoVMSNE">;
+// Special cases to avoid matching vmsltu.vi 0 (always false) to
----------------
jrtc27 wrote:
> This should be mapping to PseudoVMSLEU
done


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:4527
+defm : VPatCompare_VI<"int_riscv_vmsltu", "PseudoVMSNE">;
+// Special cases to avoid matching vmsltu.vi 0 (always false) to
+// vmsleu.vi -1 (always true). Instead match to vmsne.vv.
----------------
jrtc27 wrote:
> Isn't it a bit dodgy for this to be done in the first place; shouldn't we be stopping the normal int_riscv_vmsltu pattern from matching that at all? Currently this relies in SelectionDAG picking the more specific pattern, but that should solely be as a heuristic for code quality, not correctness.
I think what you said makes sense, but this is not something that this patch considers.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td:4527
+defm : VPatCompare_VI<"int_riscv_vmsltu", "PseudoVMSNE">;
+// Special cases to avoid matching vmsltu.vi 0 (always false) to
+// vmsleu.vi -1 (always true). Instead match to vmsne.vv.
----------------
craig.topper wrote:
> Chenbing.Zheng wrote:
> > jrtc27 wrote:
> > > Isn't it a bit dodgy for this to be done in the first place; shouldn't we be stopping the normal int_riscv_vmsltu pattern from matching that at all? Currently this relies in SelectionDAG picking the more specific pattern, but that should solely be as a heuristic for code quality, not correctness.
> > I think what you said makes sense, but this is not something that this patch considers.
> I think we did this to match the assembler where we alias vmsltu.vi and vmsgeu.vi and with 0 immediate to vmsne and vmseq.
> 
> For vmsleu.vi I suppose we could block the 0 immediate from matching in isel and let vmsleu.vx with X0 match.
> 
> That wouldn't work for vmsgeu.vi though because there is no vmsgeu.vx instruction. We use a multiple instruction sequence. But since we know this is always true we wouldn't want to use a multiple instructions. Using vmset.m(for unmasked) and vmand.mm(for masked) would be better and avoid a false dependency I guess.
> 
> But I think any change here should not be part of this patch.
Thanks! I will implement your suggestion in my next patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116215/new/

https://reviews.llvm.org/D116215