[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:8275
+
+  if (!VT.isVector())
+    return SDValue();
----------------
craig.topper wrote:
> 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.
Done.


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


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


================
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 &&
----------------
craig.topper wrote:
> 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?
Done.


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


================
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:
> 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.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131551