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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Analysis/CostModel.cpp:196
+
+static bool matchPairwiseReductionAtLevel(Value *V, unsigned Level,
+                                          unsigned NumLevels) {
----------------
mkuper wrote:
> Can this ever be a Value that's not an Instruction?
> (Same question applies to getReductionOpcode())
Of course no, only instructions can be used here. I'll convert these params to `Instruction *`.


================
Comment at: lib/Analysis/CostModel.cpp:211
+  Value *R;
+  unsigned Opcode = getReductionOpcode(I, L, R);
+  if (!Opcode)
----------------
Ayal wrote:
> Can PatternMatch be put to more use, by asking it to match a BinOp whose L and R sides are both shuffles, when Level > 0?
> 
> Passing L and R by reference here saves little compared to the original direct calls to getOperand(0) and getOperand(1), and seems mostly redundant elsewhere.
This is just an initial patch. In future it will be extended with support of `min/max` patterns, that's why this kind of `recognition` of LHS/RHS operands is placed in function. Another thing, that it will be better just to return the record of opcode + LHS/RHS parts, rather than pass L/R by reference.


================
Comment at: lib/Analysis/CostModel.cpp:405
 
-    RdxOp = NextRdxOp;
+    RdxOp = dyn_cast<Instruction>(NextRdxOp);
     NumVecElemsRemain /= 2;
----------------
Ayal wrote:
> Check if RdxOp, or use cast instead of dyn_cast?
Yes, missed this, thanks. Will fix.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4094
 
+  if (!isa<BinaryOperator>(V))
+    return false;
----------------
Ayal wrote:
> V is-surely-a<BinaryOperator>, being a non-nullptr to BinaryOperator, right?
Yes, forgot to change type of parameter, will do it


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4094
 
-  Value *P = V->getParent();
+  if (!isa<BinaryOperator>(I) && !isa<CmpInst>(I))
+    return false;
----------------
mkuper wrote:
> 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?
Yes, you're right, I'll remove these changes from this patch and prepare another one for CmpInsts.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4197
+  /// validity.
+  struct OperationData {
+    /// true if data is valid and represents one of the instructions.
----------------
mkuper wrote:
> 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.
No, actually it is basic infrastructure for supporting horizontal reduction vectorization of not-BinOp instructions. It does not change the functionality, but allows later easily to support vectorization of other complex instructions, like `min/max`.


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


================
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);
----------------
mkuper wrote:
> only adds at this point. :-)
> Also, if you're checking the op here, I'd expect a check of isAssociative() as well.
Yes, will fix this.
`isAssociative()` requires actual instruction instead of Opcode only, that's why I did not check it here.


================
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) {
----------------
mkuper wrote:
> 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?
Yes, it is exactly what I meant.
Ok, will think about it.


================
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);
----------------
mkuper wrote:
> I've cleaned this up in the existing code, there's no need to special-case FAdd, you can just CreateBinOp for it.
Ok, will fix it.


https://reviews.llvm.org/D29402