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

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck marked 5 inline comments as done.
frasercrmck added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:436
 
     static unsigned IntegerVPOps[] = {
+        ISD::VP_ADD,         ISD::VP_SUB,         ISD::VP_MUL,
----------------
craig.topper wrote:
> I just pushed a change to add 'const' to these arrays so this will need to be rebased. Sorry.
No worries, thanks for the heads up.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:3941
+  ISD::CondCode CC;
+  unsigned BaseOpc = 0;
   SDValue Zero = DAG.getConstant(0, DL, XLenVT);
----------------
craig.topper wrote:
> If you're going to initialize this, which I'm not sure you need to, use ISD::DELETED_NODE.
Yeah I don't think I need to initialize this; `CC` isn't, after all. I'll keep `ISD::DELETED_NODE` in mind though.


================
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.
----------------
craig.topper wrote:
> (0 != 0) -> 0 surely
> 
> Also the parentheses are inconsistent between (0 == 0 -> 1) and (0 != 0) -> 1
After some extensive verification -- not to mention soul searching -- yes, `0 != 0` is indeed `0`. Luckily it's just a typo and we do indeed want to see `0` as it's the neutral value for OR/XOR. Thanks for catching my silly mistake.

I've made the parens consistent too. `(0 == 0) -> 1` looked better to me at the time of making the change.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107657