[PATCH] D94168: [RISCV] Add scalable vector icmp ISel patterns

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:9086
+      assert(CVT.bitsGE(VecEltVT) && "Illegal splat_vector element extension");
+      if (AllowTruncation || (CVT.bitsEq(VecEltVT)))
+        return CN;
----------------
craig.topper wrote:
> Any special reason this bitsEq but the build_vector code is operator==?
Err no, I think that's a remnant of me trying something else out. I'll fix that up.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVSDPatterns.td:232
+// 12.8. Vector Integer Comparison Instructions
+defm "" : VPatIntegerSetCCSDNode_VV_VX_VI<SETEQ,  "PseudoVMSEQ">;
+defm "" : VPatIntegerSetCCSDNode_VV_VX_VI<SETNE,  "PseudoVMSNE">;
----------------
craig.topper wrote:
> frasercrmck wrote:
> > craig.topper wrote:
> > > What is the splat for VX/VI is on the LHS?
> > Ah, good spot. I had assumed constants were canonicalized to the RHS like they are with scalar/fixed-length vectors.
> > 
> > I've now updated `llvm::isConstOrConstSplat` to handle `SPLAT_VECTOR` and it "just works". I also see further optimization such as for `uge v, 0`:
> > 
> > ```
> > -; CHECK-NEXT:    vmv.v.i v25, 0
> > -; CHECK-NEXT:    vmsleu.vv v0, v25, v16
> > +; CHECK-NEXT:    vmset.m v0
> > ```
> > 
> > That doesn't solve the problem for VX though. I'd probably suggest a custom combine for that, to avoid too much pattern complication. Any thoughts about that?
> There's a small possibility of an infinite loop if the generic combines don't know the rule being used. They might end up converting it back and forth. For example, there's a combine that tries to canonicalize setcc with non-constant operands to have the same operand order as an existing isd::sub. Which probably only make sense for scalars where sub produces flags on some targets.
> 
> Can you add a FIXME for VX LHS? I think this patch can go in and we can address it as follow up.
Yeah good point. I had woken up today hoping we could just do it in the same place as the constant canonicalization with a target hook (`prefersSplatOnRHS(Opcode, Operand)` or something) if AArch64 would also benefit from it. It appears they're doing it manually in patterns. It might still reduce their pattern load, but it has taken the impetus out of that idea somewhat.

But for now, I'll enhance the tests and add a FIXME to them like I did with the fp tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94168