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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev marked an inline comment as not done.
ABataev 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);
----------------
Ayal wrote:
> 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?
Added isVectorized(Instruction *I)


================
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:
> 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?
Modified it a bit, still need to compare the opcode


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4094
 
+  if (!isa<BinaryOperator>(V))
+    return false;
----------------
Ayal wrote:
> 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.
Restored back.


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


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4269
+    Value *createOp(IRBuilder<> &Builder, Value *L, Value *R,
+                    const Twine &Name = "") const {
+      assert(!IsReducedValue &&
----------------
Ayal wrote:
> 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?
Modified this method, now it accepts only Builder and optional name


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4262
+  /// Contains info about operation, like its opcode, left and right operands +
+  /// validity.
+  struct OperationData {
----------------
Ayal wrote:
> "+ validity" should be removed?
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.
----------------
Ayal wrote:
> Still a couple of "if (x.Opcode == 0)" cases, may as well make use of this bool().
Tried to replace all


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


https://reviews.llvm.org/D29402