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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
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) {
----------------
ABataev wrote:
> 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.
The IsReducedValue explicit flag indicates if its a reduction op or reduced value, right?

To implement the behavior documented above: "Checks if two operation data are both a reduction op or both a reduced value", it's enough to have the "IsReducedValue == OD.IsReducedValue" part.


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4213
+      if (auto *I = dyn_cast<Instruction>(V))
+        Opcode = I->getOpcode();
+    }
----------------
Should IsReducedValue be set to false here?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4267
+    Value *getRHS() const { return RHS; }
+    /// Creates reuction operation with the current opcode.
+    Value *createOp(IRBuilder<> &Builder, Value *L, Value *R,
----------------
re[d]uction


https://reviews.llvm.org/D29402