This is an archive of the discontinued LLVM Phabricator instance.

[LoopVectorizer] Don't create unused block masks for reductions. NFC
ClosedPublic

Authored by dmgreen on Jun 8 2020, 11:35 AM.

Details

Summary

This is pulled out of D75069, where I somewhat accidentally removed some unneeded block masks when we don't have any reductions. This is the same thing as a separate patch, and shouldn't have any effect on codegen.

Diff Detail

Event Timeline

dmgreen created this revision.Jun 8 2020, 11:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2020, 11:35 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
SjoerdMeijer added inline comments.Jun 23 2020, 2:41 AM
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1324

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.

dmgreen marked an inline comment as done.Jun 23 2020, 4:34 AM
dmgreen added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1324

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.

SjoerdMeijer accepted this revision.Jun 23 2020, 6:15 AM
SjoerdMeijer added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
1324

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.

This revision is now accepted and ready to land.Jun 23 2020, 6:15 AM
dmgreen updated this revision to Diff 278680.Jul 17 2020, 2:06 AM

Update this to be a little simpler, not including hasOutOfLoopReductions which turned out to not be needed in the followup.