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

Michael Kuperstein via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Feb 1 15:24:46 PST 2017

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.


More information about the llvm-commits mailing list