[PATCH] D27846: [SLP] Support for horizontal min/max reduction

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added inline comments.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:1182
+    unsigned CmpOpcode;
+    if (Ty->getVectorElementType()->isFloatingPointTy())
+      CmpOpcode = Instruction::FCmp;
----------------
ScalarTy->isFloatingPointTy()


================
Comment at: lib/Analysis/CostModel.cpp:194
+  MinMaxReduction,     /// Min/max reduction data.
+};
 /// Contains opcode + LHS/RHS parts of the reduction operations.
----------------
They're not public, but maybe keep to style guide (also, maybe drop the class?)
```
enum ReductionKind {
  RK_None,        /// Not a reduction.
  RK_Arithmetic, /// Binary reduction data.
  RK_MinMax,     /// Min/max reduction data.
};
```


================
Comment at: lib/Analysis/CostModel.cpp:210
+  bool hasSameData(ReductionData &RD) const {
+    return this == &RD || (Kind == RD.Kind && Opcode == RD.Opcode);
+  }
----------------
Do you need the this == &RD? Won't it always match on (Kind == RD.Kind && Opcode == RD.Opcode)?


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1892
+  static const CostTblEntry SSE42CostTblPairWise[] = {
+      {ISD::FMINNUM, MVT::v2f64, 3}, {ISD::FMINNUM, MVT::v4f32, 2},
+      {ISD::SMIN, MVT::v2i64, 7}, // The data reported by the IACA is "6.8"
----------------
One cost entry per line


================
Comment at: lib/Transforms/Vectorize/SLPVectorizer.cpp:4501
+    UMaxReduction,       /// Unsigned maximum reduction data.
+  };
   /// Contains info about operation, like its opcode, left and right operands.
----------------
Same as above:
```
  enum ReductionKind {
    RK_Not,        /// Not a reduction.
    RK_Arithmetic, /// Binary reduction data.
    RK_Min,        /// Minimum reduction data.
    RK_UMin,       /// Unsigned minimum reduction data.
    RK_Max,        /// Maximum reduction data.
    RK_UMax,       /// Unsigned maximum reduction data.
  };
```


https://reviews.llvm.org/D27846