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

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon added a comment.

A few comments, but someone with more vectorizer knowledge needs to review as well.



================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:1009
+    else {
+      assert(Ty->isIntOrIntVectorTy());
+      CmpOpcode = Instruction::ICmp;
----------------
Missing assert message


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:1012
+    }
+    // Try to calculate arithmetic and shuffle op costs for reduction operations.
+    // We're assuming that reduction operation are performing the following way:
----------------
Move this comment out and just above the arithmetic/minmax functions?


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:1020
+    // %red1 = op <n x t> %val, <n x t> val1
+    // After this operation we have a vector %red1 with only maningfull the
+    // first n/2 elements, the second n/2 elements are undefined and can be
----------------
where only the first n/2 elements are meaningful,


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:1023
+    // dropped. All other operations are actually working with the vector of
+    // length n/2, not n. though the real vector length is still n.
+    // %val2 = shufflevector<n x t> %red1, <n x t> %undef,
----------------
, not n, 


================
Comment at: lib/Target/X86/X86TargetTransformInfo.cpp:1659
+  int ISD = ValTy->isIntOrIntVectorTy() ? ISD::SMIN : ISD::FMINNUM;
+  assert(ISD && "Invalid opcode");
+
----------------
Unnecessary?


https://reviews.llvm.org/D27846