[PATCH] D78210: [LV] Mark first-order recurrences as allowed exit

[Florian Hahn via Phabricator via llvm-commits]

fhahn accepted this revision.
fhahn added a comment.

LGTM, thanks!



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:671
                                                          SinkAfter, DT)) {
+          AllowedExit.insert(Phi);
           FirstOrderRecurrences.insert(Phi);
----------------
Ayal wrote:
> fhahn wrote:
> > nit: it seems a bit counter-intuitive to add to something called `AllowedExit` to reject loops with users of Phi outside the loop. There might be potential for re-naming/refactoring to make it clearer in future. In the meantime it might make sense to add a comment.
> Not sure what comment may be worth adding where. AllowedExit was introduced originally to record all exits that can be vectorized, and first-order-recurring phi's should have been included in it when support for vectorizing them was first introduced (in D16197, as noted in the summary).
> AllowedExit is examined by prepareToFoldTail because only a subset of generally vectorizable exits are also supported when vectorizing with foldTail. Perhaps rename `AllowedExit` to `AllowedGeneralExit` ?
> Above TODO was added to revisit the use of AllowedExit in general.
> Above TODO was added to revisit the use of AllowedExit in general.

Right, I think the existing TODO is fine.


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

https://reviews.llvm.org/D78210