[PATCH] D30086: Add generic IR vector reductions

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1595
+    VecOpIdx = 1;
+  assert(OpNo == VecOpIdx && "Can only split reduce vector operand");
+
----------------
Wouldn't this assert be better to check on the type of the operand?


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1
+
 //===-- SelectionDAGBuilder.cpp - Selection-DAG building ------------------===//
----------------
unnecessary whitespace change


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGDumper.cpp:352
+  case ISD::VECREDUCE_AND:             return "vecreduce_and";
+  case ISD::VECREDUCE_OR:             return "vecreduce_or";
+  case ISD::VECREDUCE_XOR:             return "vecreduce_xor";
----------------
format?


================
Comment at: lib/Transforms/Utils/LoopUtils.cpp:1191
+
+Value *llvm::createTargetReduction(IRBuilder<> &Builder,
+                                   const TargetTransformInfo *TTI,
----------------
aemerson wrote:
> delena wrote:
> > You implemented 2 functions with the same name. It's not clear what the difference between them. Please add some comments before each of them.
> I've added some comments to the API in the header, but I'll flesh those out more and add some short ones here too.
I think they're different enough that they deserve different names. Normally, you should use the same name if the arguments are different, but the functionality is the same. Here, the implementation and the logic are wildly different that naming them the same thing could be misleading.

I haven't looked deep to know if there is SLP logic here that could have remained in the SLP vectorizer's code, or if you could separate the implementation where one is just a wrapper to the other. This would make it more clear, and probably easier to maintain.


Repository:
  rL LLVM

https://reviews.llvm.org/D30086