[PATCH] D115757: [SLP]Generalize cost model.

[Valeriy Dmitriev via Phabricator via llvm-commits]

vdmitrie added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/SLPVectorizer.cpp:6313
+          ScalarCost = VL.size() *
+                       ScalarEltCost(std::distance(VL.begin(), find(VL, VL0)));
+        } else {
----------------
ABataev wrote:
> RKSimon wrote:
> > ABataev wrote:
> > > RKSimon wrote:
> > > > Is this purely compile time saving?
> > > Replaced by 0, since it does not matter.
> > I'm not sure if we need to pull them out into separate methods if they're just being called once. But it would help a little for readability if the lambdas weren't embedded in the GetCostDiff calls themselves.
> > 
> > So:
> > ```
> > auto ScalarEltCost = [](unsigned Idx)  {...};
> > auto VectorCost = [](InstructionCost CommonCost)  {...};
> > return GetCostDiff(ScalarEltCost, VectorCost);
> > ```
> I prefer this approach. I don't want to introduce virtual member functions. Without virtual functions the only approach - generic lambda, which increases codesize.
What problem you see with virtual class here?
Using named lambdas instead of unnamed ones is definitely better. But essentially it anyway remains a C-style programming,  just using C++ syntax. You are also making it very difficult to customize this code downstream. If that is the goal - okay, got it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115757