[PATCH] D115546: [RISCV][VP] Add RVV codegen for [nX]vXi1 vp.select

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1241
+SDValue VectorLegalizer::ExpandVP_SELECT(SDNode *Node) {
+  // Implent VP_SELECT in terms of VP_XOR, VP_AND and VP_OR on platforms which
+  // do not support it natively.
----------------
frasercrmck wrote:
> nit: `Implement`
This code only works if Mask's VT is the same as Op1/Op2 VT but that isn't guaranteed in general. We should call UnrollVectorOp for that just like ExpandVSELECT.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1258
+    return DAG.UnrollVectorOp(Node);
+
+  auto NotMask = [&]() {
----------------
frasercrmck wrote:
> The following expansion is only correct under certain conditions: when the vselect condition and operand types are identically-sized integer vectors and when the mask is -1/0. See the original `ExpandVSELECT` for that.
> 
> Since we don't have an in-tree test case for anything other than expanding [nx]vXi1 VSELECT maybe it's sufficient for now to resort to `UnrollVectorOp` whenever the operand types aren't i1 vectors? `UnrollVectorOp` will no doubt fail for `VP_SELECT` right now, but we wouldn't be making anything worse.
> 
> Not sure: any thoughts @craig.topper? @RKSimon?
I think I'm fine with calling Unroll if it isn't an nxvXi1 vector.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1259
+
+  auto NotMask = [&]() {
+    auto Ones = DAG.getAllOnesConstant(DL, VT);
----------------
Why a lamdba?  Can't we just do

```
SDValue Ones = DAG.getAllOnesConstant(DL, VT);
SDValue NotMask = DAG.getNode(ISD::VP_XOR, DL, VT, Mask, Ones, Ones, Length);
```

Arguably that should be 
```
DAG.getNode(ISD::VP_XOR, DL, VT, Mask, Ones, Mask, Length);
```
so the XOR has the same mask as the other ops.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115546