[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:4245
+      return Validity && LHS && RHS &&
+             // We currently only support adds and min/max.
+             (Opcode == Instruction::Add || Opcode == Instruction::FAdd);
----------------
ABataev wrote:
> 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.
isVectorizable() is used only in conjuction with isAssociative(I), and as an assert when getOpcode() is called by getReductionCost(). Isn't it better to have isVectorizable(I) also call isAssociative?


================
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) {
----------------
Ayal wrote:
> 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.
It sounds like you can

```
  assert ((IsReducedValue != OD.IsReducedValue) || ((!LHS == !OD.LHS) && (!RHS == !OD.RHS) && Opcode == OD.Opcode) && "message");

```
no need to actually check them all to return true, right?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4094
 
+  if (!isa<BinaryOperator>(V))
+    return false;
----------------
ABataev wrote:
> Ayal wrote:
> > V is-surely-a<BinaryOperator>, being a non-nullptr to BinaryOperator, right?
> Yes, forgot to change type of parameter, will do it
I must be missing something here; tryToVectorize() is still being fed by BinaryOperator*, right? Wondering why the parameter type change. Better include this change in follow-up patch which tries to vectorize non BinaryOperators, if that's the intention.


================
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,
----------------
Ayal wrote:
> re[d]uction
typo


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4269
+    Value *createOp(IRBuilder<> &Builder, Value *L, Value *R,
+                    const Twine &Name = "") const {
+      assert(!IsReducedValue &&
----------------
Every other method fits with the passive, storage-type "OperationData" name of this struct, but this last method may look a bit odd doing "OperationData.createOp()". All that it's contributing is its Opcode which it can provide, and some asserts. Is there a better way of keeping OperationData's nature, or a better name for it?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4262
+  /// Contains info about operation, like its opcode, left and right operands +
+  /// validity.
+  struct OperationData {
----------------
"+ validity" should be removed?


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4285
+        : IsReducedValue(false), Opcode(Opcode), LHS(LHS), RHS(RHS) {}
+    operator bool() const { return Opcode; }
+    /// Get the index of the first operand.
----------------
Still a couple of "if (x.Opcode == 0)" cases, may as well make use of this bool().


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4865
+      auto *BI = dyn_cast<BinaryOperator>(Inst);
+      if (BI) {
         HorizontalReduction HorRdx;
----------------
Not sure why you wanted this change; but how about simply "if (isa<BinaryOperator>(Inst))"?


https://reviews.llvm.org/D29402