[PATCH] D59149: [LV] move useEmulatedMaskMemRefHack() functionality to TTI.

[Renato Golin via Phabricator via llvm-commits]

rengolin added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:5488
 
-    if (useEmulatedMaskMemRefHack(I))
+    if (int HackedCost = TTI.getEmulatedMaskMemRefCost(
+            I, NumPredStores, NumberOfStoresToPredicate))
----------------
markus wrote:
> rengolin wrote:
> > hsaito wrote:
> > > rengolin wrote:
> > > > This if doesn't need the Cost above, so you can avoid the division by moving it up to the beginning of the block.
> > > > 
> > > > Nit: you can just return `HackedCost` instead of assigning, but that will depend on the new pattern.
> > > This is actually a good question.
> > > 
> > > Maybe, we want to structure this TTI interface as the adjustment to the base cost computed above.
> > > In which case, the code would look like
> > >    Cost += TTI.getEmulatedMaskMemRefAdjustment(...)
> > > 
> > > Does this make more sense?
> > > 
> > > Even then, I still agree that we can return early like
> > > if (!isPredicaredInst(I))
> > >      return Cost;
> > > Cost /= getReciprocalPredBlockProb();
> > > return Cost + TTI.getEmulatedMaskMemRefAdjustment(...)
> > > 
> > > Does this sound better?
> > If the prohibitive cost is large enough, then adding up will make no difference, and you can early return on either Cost or Adjusted.
> > 
> > If the cost isn't meant as just "big enough" but to (later) actually emulate the masking costs, then it makes sense to add like you propose on the comment above.
> > 
> > Right now, the case looks like the former, but if you're planning it to be more like the latter, than the new proposal makes sense.
> I agree that this should be changed to
> ```
> Cost += TTI.getEmulatedMaskMemRefAdjustment(...)
> ```
> (or equivalent with the early return).
IFF the adjustment has any meaning other than "too large a number", I agree, too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59149