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

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added a comment.

I'm sorry for the delay. Was bit overloaded with an internal stuff.



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12238
             !hasRequiredNumberOfUses(IsCmpSelMinMax, EdgeInst) ||
             !isVectorizable(getRdxKind(EdgeInst), EdgeInst)) {
           PossibleReducedVals.push_back(EdgeVal);
----------------
nit: noticed by chance while was refreshing with the code. This call to getRdxKind can be safely replaced with RdxKind.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12539-12548
+        if (!VectorizedTree) {
+          // Initialize the final value in the reduction.
+          VectorizedTree = Res;
+        } else {
+          // Update the final value in the reduction.
+          Builder.SetCurrentDebugLocation(
+              cast<Instruction>(ReductionOps.front().front())->getDebugLoc());
----------------
This code pattern is seen multiple times. Any chance to outline it?
+ (for better readability)  swap then/else and remove '!' from condition?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12560
+      MapVector<Value *, unsigned> SameValuesCounter;
+      for (Value *V : Candidates)
+        ++SameValuesCounter.insert(std::make_pair(V, 0)).first->second;
----------------
We don't need to collect repeating counters if IsSupportedreusedRdxOp is false.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12564-12565
+      bool IsSupportedReusedRdxOp =
+          AllowSameScalarsReduction && RdxKind != RecurKind::Mul &&
+          RdxKind != RecurKind::FMul && RdxKind != RecurKind::FMulAdd;
+      bool SameScaleFactor = false;
----------------
This code shall form a member of HorizontalReduction class and emitReusedOps()  need to assert it can handle a case by calling the former before doing any processing.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12566
+          RdxKind != RecurKind::FMul && RdxKind != RecurKind::FMulAdd;
+      bool SameScaleFactor = false;
+      bool HasReusedScalars = SameValuesCounter.size() != Candidates.size();
----------------
This needs a comment. Are you trying to catch a case for special processing when each value used same times? Please describe what is the benefit of catching such cases?
Why are you trying to calculate it this early?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12572
+             RdxKind == RecurKind::Xor) &&
+            all_of(SameValuesCounter,
+                   [&SameValuesCounter](const std::pair<Value *, unsigned> &P) {
----------------
drop_front() ?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12685
+          // Number of uses of the candidates in the vector of values.
+          SameValuesCounter.clear();
+          for (unsigned Cnt = 0; Cnt < Pos; ++Cnt) {
----------------
Are you trying to repurpose the data?
To be honest, I'd refrain from doing that.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12686-12695
+          for (unsigned Cnt = 0; Cnt < Pos; ++Cnt) {
+            Value *V = Candidates[Cnt];
+            Value *OrigV = TrackedToOrig.find(V)->second;
+            ++SameValuesCounter[OrigV];
+          }
+          for (unsigned Cnt = Pos + ReduxWidth; Cnt < NumReducedVals; ++Cnt) {
+            Value *V = Candidates[Cnt];
----------------
Fuse these loops?

 ` for (unsigned Cnt = 0; Cnt < NumReducedVals; ++Cnt) {
     if (Cnt >= Pos && Cnt < Pos + ReduxWidth)
       continue;
    ...
  }`



================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12700-12731
         for (unsigned Cnt = 0; Cnt < Pos; ++Cnt) {
           Value *RdxVal = Candidates[Cnt];
           if (!Visited.insert(RdxVal).second)
             continue;
           // Check if the scalar was vectorized as part of the vectorization
           // tree but not the top node.
           if (!VLScalars.contains(RdxVal) && V.isVectorized(RdxVal)) {
----------------
Fuse these loops?


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:13109
+
+  Value *emitReusedOps(Value *VectorizedValue, IRBuilderBase &Builder,
+                       ArrayRef<Value *> VL,
----------------
Please add a description.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:131
+    "slp-optimize-identity-hor-reduction-ops", cl::init(true), cl::Hidden,
+    cl::desc("Allow horizontal reduction of same scalars."));
+
----------------
Would the following description 
"Allow optimization of original scalar identity operations on matched horizontal reductions."
sound better?
Same question wrt AllowSameScalarReduction  variable name. Will AllowHorRdxIdenityOptimization fit better?

It probably makes sense to add a note (as a comment above or into the commit message): will the optimization run if we match a reduction but do not vectorize at the end? Based on test updates it looks like it will run. IMO that should be clearly stated that the pass outcome might not be a vectorized code.







================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12416
         std::accumulate(ReducedVals.begin(), ReducedVals.end(), 0,
                         [](size_t Num, ArrayRef<Value *> Vals) {
                           if (!isGoodForReduction(Vals))
----------------
nit: maybe make it unsigned too (+ deduction rule)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132261