[PATCH] D81415: [LoopVectorizer] Don't create unused block masks for reductions. NFC

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1306
+  bool hasOutOfLoopReductions() const {
+    return !Legal->getReductionVars().empty();
+  }
----------------
dmgreen wrote:
> SjoerdMeijer wrote:
> > Quick question on terminology and "inside" and "outside".
> > `getReductionVars()` returns "reduction variables found in the loop". Here you're actually checking if there are inside loop reductions. But having inside reductions means there's a statement/loop after the vectorised loop that sums the partial reductions/sums, correct? If so, it's probably worth clarifying this a bit.  
> Ideally this would go with D75069, where inloop vs outside loop reductions are introduced. Until then this might not be the best language on it's own, but I was planning to commit the two close together in any case.
> 
> Outside loop reductions are what you would expect from reductions right now:
>   loop:
>     l = load
>     a = add a, l
>   vecreduce(a)
> 
> Inside loop reductions put the reduction into the loop:
>   loop:
>     l = load
>     a = a + vecreduce(l)
> 
> So in the short term, this should probably be called "hasReductions()".  But with D75069 should make more sense.
Thanks for clarifying. I got it, and yes, together with D75069 it makes sense.
"in-loop reduction" is descriptive, while I am not entirely sold on "outside loop reductions". I was used to terms "partial reductions" and "final reduction", where the latter collects the partial values, which is done after/outside the loop. But we are possibly entering bikeshedding territory here.  This looks like a good fix, with `hasOutOfLoopReductions` renamed to `hasReductions` like you suggested, or possibly `hasFinalReduction` if that makes sense.




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

https://reviews.llvm.org/D81415