[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:12568
+      bool HasReusedScalars = SameValuesCounter.size() != Candidates.size();
+      if (IsSupportedReusedRdxOp && HasReusedScalars) {
+        SameScaleFactor =
----------------
"IsSupportedReusedRdxOp && HasReusedScalars" pattern is the most common use.
What about defining it as below
 `bool OptReusedScalars = IsSupportedReusedRdxOp &&
           SameValuesCounter.size() != NumReducedVals;`
and just check for OptReusedScalars everywhere across the code?
It only used differently at line 12683 but check for HasReusedScalars can be safely dropped there.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:12572
+             RdxKind == RecurKind::Xor) &&
+            all_of(SameValuesCounter,
+                   [&SameValuesCounter](const std::pair<Value *, unsigned> &P) {
----------------
vdmitrie wrote:
> vdmitrie wrote:
> > drop_front() ?
> > drop_front() ?
> 
> This was a "nit" actually. drop_front() needs ArrayRef, so it should look like the following:
> ArrayRef<std::pair<Value *, unsigned>>(SameValuesCounter.takeVector()).drop_front()
> 
> probably not worth the effort to save on just one comparison. 
Uhh, I did not realize takeVector clears the map.
finally... this one should do the trick:

`all_of(drop_begin(SameValuesCounter),
                   [&SameValuesCounter](const std::pair<Value *, unsigned> &P) {
                     return P.second == SameValuesCounter.begin()->second;
                    })`


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:13213
+    }
+  }
 };
----------------
May be reorganize it a bit to add a return statement? It can produce warning "control reaches end of non-void function [-Wreturn-type]"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132261