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

[Jianjian Guan via Phabricator via llvm-commits]

jacquesguan added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:981
+                         ISD::UREM, ISD::SREM});
+  if (Subtarget.hasVInstructions() && !Subtarget.hasStdExtZbb())
+    setTargetDAGCombine({ISD::UMIN, ISD::UMAX});
----------------
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`.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8290
+  if (N0.getOpcode() == ISD::VSELECT) {
+    N0 = N->getOperand(1);
+    N1 = N->getOperand(0);
----------------
craig.topper wrote:
> std::swap?
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8324
+    OP_CASE(ADD, 0)
+    OP_CASE(SUB, 0)
+    OP_CASE(MUL, 1)
----------------
craig.topper wrote:
> Some of these operations aren't commutable. If the select is on the LHS of the SUB this transform isn't valid.
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8326
+    OP_CASE(MUL, 1)
+    OP_CASE(MULHS, 1)
+    OP_CASE(MULHU, 1)
----------------
craig.topper wrote:
> 1 is also not the identify value for MULHS/MULHU. This returns the high half of the product. MULHU(a, 1) produces 0 since the high product is 0. MULHS(a, 1) is a vector filled with the sigh bit of a.
Done, I removed `MULHS`, `MULHU`, `UREM` and `SREM`.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8330
+    OP_CASE(SDIV, 1)
+    OP_CASE(UREM, 1)
+    OP_CASE(SREM, 1)
----------------
craig.topper wrote:
> 1 is not an identity value for urem or srem, it would always return 0.
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8347
+
+  SDValue VL =
+      DAG.getConstant(RISCV::VLMaxSentinel, SDLoc(N), Subtarget.getXLenVT());
----------------
craig.topper wrote:
> VLMaxSentinel is not to be used in SelectionDAG anymore. Use `getRegister(RISCV::X0`
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8357
+    return V;
+
   if (SDValue V = transformAddImmMulImm(N, DAG, Subtarget))
----------------
jrtc27 wrote:
> Why the blank line when the others don't have one?
Done.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:8317
+  switch (N->getOpcode()) {
+  case ISD::ADD:
+    RISCVVLISDOpc = RISCVISD::ADD_VL;
----------------
jrtc27 wrote:
> jacquesguan wrote:
> > StephenFan wrote:
> > > Is it possible to use a macro definition to reduce code lines?
> > Done.
> Undefine helper macros like this once you're done with them
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131551