[PATCH] D29402: [SLP] Initial rework for min/max horizontal reduction vectorization, NFC.

[Michael Kuperstein via Phabricator via llvm-commits]

mkuper added reviewers: gilr, Ayal.
mkuper added inline comments.


================
Comment at: lib/Analysis/CostModel.cpp:196
+
+static bool matchPairwiseReductionAtLevel(Value *V, unsigned Level,
+                                          unsigned NumLevels) {
----------------
Can this ever be a Value that's not an Instruction?
(Same question applies to getReductionOpcode())


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4094
 
-  Value *P = V->getParent();
+  if (!isa<BinaryOperator>(I) && !isa<CmpInst>(I))
+    return false;
----------------
This basically complements the change in line 4803 - 4823, right, but taken together, those two changes are NFC, regardless of everything else here, right? Or am I confused and this depends on something else?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4197
+  /// validity.
+  struct OperationData {
+    /// true if data is valid and represents one of the instructions.
----------------
This part of the change is completely orthogonal to the name change and using matchers, right?

Anyway, I think I see where this is going. This is possibly similar to what Ayal, Gil & company are doing for LV, in terms of recipes?
Adding them to take a look.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4198
+  struct OperationData {
+    /// true if data is valid and represents one of the instructions.
+    bool Validity = false;
----------------
What does being valid mean in this context? Just that this is a "non-null handle"




================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4245
+      return Validity && LHS && RHS &&
+             // We currently only support adds and min/max.
+             (Opcode == Instruction::Add || Opcode == Instruction::FAdd);
----------------
only adds at this point. :-)
Also, if you're checking the op here, I'd expect a check of isAssociative() as well.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4249
+    /// Checks if two operation data represent reduction operation/reduced
+    /// value of the same kind.
+    bool operator==(const OperationData &OD) {
----------------
So, by "of the same kind" you mean "both a reduction op or both a reduced value", which is something you determine by either both LHS/RHS being null, or both being non-null?
Can you please be more explicit about this? Also, maybe it's worth having an explicit flag for this, instead of relying on LHS and RHS being null?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4271
+      assert(Opcode == Instruction::FAdd || Opcode == Instruction::Add);
+      if (Opcode == Instruction::FAdd)
+        return Builder.CreateFAdd(L, R, Name);
----------------
I've cleaned this up in the existing code, there's no need to special-case FAdd, you can just CreateBinOp for it.


https://reviews.llvm.org/D29402