[PATCH] D131551: [RISCV] Fold scalable binary integer vector op and vector select op to RVV VL Op.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8275
+
+  if (!VT.isVector())
+    return SDValue();
----------------
This isn't protected against illegal types. DAGCombiners can see any types.

My suggestion is to defer this combine until `DCI.isAfterLegalizeDAG()` which ensure types are legalized and that we don't create _VL operations too early.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8286
+  unsigned RISCVVLISDOpc;
+  int64_t Imm;
+  bool IsCommutable;
----------------
`Imm` -> `IdentityImm`


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8293
+    Imm = IDENTIFY_VALUE;                                                      \
+    IsCommutable = IS_COMMUTABLE;                                              \
+    break;
----------------
Use `TargetLowering::isCommutativeBinOp`?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8333
+      isa<ConstantSDNode>(FalseVal.getOperand(0)))
+    SplatImm = cast<ConstantSDNode>(FalseVal.getOperand(0))->getSExtValue();
+  else if (FalseVal.getOpcode() == RISCVISD::VMV_V_X_VL &&
----------------
Technically you need to truncate the splat_vector input to element size. But maybe all your tests are before type legalization so you don't test any SPLAT_VECTORS where the scalar input is larger than the element type?


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8337
+           isa<ConstantSDNode>(FalseVal.getOperand(1)))
+    SplatImm = cast<ConstantSDNode>(FalseVal.getOperand(1))->getSExtValue();
+  else
----------------
VMV_V_X_VL is allowed to have scalar values both larger and smaller than the element type.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:981
+                         ISD::UREM, ISD::SREM});
+  if (Subtarget.hasVInstructions() && !Subtarget.hasStdExtZbb())
+    setTargetDAGCombine({ISD::UMIN, ISD::UMAX});
----------------
jacquesguan wrote:
> craig.topper wrote:
> > Why !hasStdExtZbb()?
> ```
>   if (Subtarget.hasStdExtZbb())
>     setTargetDAGCombine({ISD::UMAX, ISD::UMIN, ISD::SMAX, ISD::SMIN});
> ```
> Because if we have Zbb, we already set `ISD::UMIN` and `ISD::UMAX`.
It's harmless to do it again and would make the code less confusing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131551