[PATCH] D132261: [SLP]Do not reduce repeated values, use scalar red ops instead.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12567
 
+      // Check if we have repeated values.
+      IsSupportedHorRdxIdentityOp =
----------------
This comment line  seems to be misplaced.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:13101-13119
+      // root = mul prev_root, <1, 1, n, 1>
+      if (VL.empty()) {
+        unsigned Cnt = SameValuesCounter.lookup(
+            TrackedToOrig.find(VectorizedValue)->second);
+        Value *Scale = ConstantInt::get(VectorizedValue->getType(), Cnt);
+        LLVM_DEBUG(dbgs() << "SLP: Add (to-mul) " << Cnt << "of "
+                          << VectorizedValue << ". (HorRdx)\n");
----------------
What I basically see here is that you packed two methods into one and differentiate them by VL argument.
May be just split this method in two ?  One to handle identity optimization and another for same scale factor optimization. They seems do not share any essential code, only the switch. But sharing the switch does not look right to me as these optimizations do not fully share reduction kinds.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:13181
+                          << VectorizedValue << ". (HorRdx)\n");
+        return Builder.CreateFMul(VectorizedValue, Scale);
+      }
----------------
Are you relying on instcombiner to optimize this further? I mean for example mul X, 2 -> add X, X


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D132261/new/

https://reviews.llvm.org/D132261