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

[Craig Topper via Phabricator via llvm-commits]

craig.topper 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;
----------------
Any special reason this bitsEq but the build_vector code is operator==?


================
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">;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94168