[PATCH] D107657: [RISCV][VP] Add support for VP_REDUCE_* operations

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3941
+  ISD::CondCode CC;
+  unsigned BaseOpc = 0;
   SDValue Zero = DAG.getConstant(0, DL, XLenVT);
----------------
If you're going to initialize this, which I'm not sure you need to, use ISD::DELETED_NODE.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3985
+  // 0 for an inactive vector, and so we've already received the neutral value:
+  // AND gives us (0 == 0 -> 1) and OR/XOR give us (0 != 0) -> 1. Therefore we
+  // can simply include the start value.
----------------
(0 != 0) -> 0 surely

Also the parentheses are inconsistent between (0 == 0 -> 1) and (0 != 0) -> 1


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td:602
+    def: Pat<(vti_m1.Vector (vop (vti_m1.Vector VR:$merge), (vti.Vector vti.RegClass:$rs1), VR:$rs2,
+                                 (vti.Mask VMV0:$vm),
+                                 VLOpFrag)),
----------------
Use (vti.Mask V0)


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoVVLPatterns.td:608
+            (vti_m1.Vector VR:$rs2),
+            VMV0:$vm, GPR:$vl, vti.Log2SEW)>;
   }
----------------
Use (vti.Mask V0) in place of VMV0:$vm


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107657