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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal marked 2 inline comments as done.
Ayal added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:671
                                                          SinkAfter, DT)) {
+          AllowedExit.insert(Phi);
           FirstOrderRecurrences.insert(Phi);
----------------
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.


================
Comment at: llvm/test/Transforms/LoopVectorize/optsize.ll:129
+; CHECK-LABEL: @pr45526
+; CHECK-NOT: <
+;
----------------
fhahn wrote:
> This seems like a very subtle way to check vectorization is not happening. Given that the function is small, it might be better to check the IR for the whole function (or just the control-flow)
ok, replacing this CHECK-NOT with a positive check that the IR remains intact, which is small enough in this case.
Other tests check non-vectorization by CHECK_NOT's of specific vector types or of vector.body.


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

https://reviews.llvm.org/D78210