[PATCH] D26277: [SLP] Fixed cost model for horizontal reduction.

[Michael Kuperstein via llvm-commits]

mkuper added a comment.

Thanks for catching this Alexey, computing the scalar reduction cost with the vector type does look very odd.



================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:928
     unsigned NumVecElts = Ty->getVectorNumElements();
     unsigned NumReduxLevels = Log2_32(NumVecElts);
+    unsigned ArithCost = 0;
----------------
The computation is getting pretty hairy. Could you please add comments explaining what exactly it computes?

I understand you're trying to model the reduction steps performed on illegal vectors and steps performed on legal steps separately, but I'm not sure this computes precisely what we want.


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:932
+    std::pair<unsigned, MVT> LT =
+        static_cast<T *>(this)->getTLI()->getTypeLegalizationCost(DL, Ty);
+    unsigned I = 0;
----------------
Maybe save static_cast<T *>(this) in a temporary, for readability's sake, now that you're adding more uses?


================
Comment at: include/llvm/CodeGen/BasicTTIImpl.h:934
+    unsigned I = 0;
+    for (I = 0; LT.first > 2; ++I) {
+      NumVecElts /= 2;
----------------
I counts the number of illegal reduction steps, right?
Maybe give it a more meaningful name? It's not a regular loop counter where it's obvious what it counts up to.


https://reviews.llvm.org/D26277