[PATCH] D67074: [LV] Fix miscompiles by adding non-header PHI nodes to AllowedExit

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a subscriber: anna.
Ayal added a comment.

This LGTM too; added a couple of minor comments about comments.



================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorizationLegality.h:449
+  /// outside the loop (including induction and reduction vars and non-header
+  /// phis).
   SmallPtrSet<Value *, 4> AllowedExit;
----------------
This BTW also includes non-phi instructions, as of rL340278 thanks to @anna. As indicated in a TODO below, perhaps it's better to specify what's not included. For now, perhaps it's better to simply avoid specifying what's included.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:779
       // Reduction instructions are allowed to have exit users.
       // All other instructions must not have external users.
       if (hasOutsideLoopUser(TheLoop, &I, AllowedExit)) {
----------------
While we're at it, the above comment deserves updating as well..


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:785
         // outside the loop.
         if (PSE.getUnionPredicate().isAlwaysTrue()) {
           AllowedExit.insert(&I);
----------------
(BTW, this SCEV-predicates condition, related to PR33706, should apply only to IV header-phi's and related instructions. But it's conservative; relaxing it calls for a separate optimization fix.)


================
Comment at: llvm/test/Transforms/LoopVectorize/lv-fold-tail-by-masking-bug.ll:36
+; Used to miscompile (with clang 8.0.0), now we get
+;   loop not vectorized: Cannot fold tail by masking in the presence of live outs.
 define i64 @test1(i64 %y) {
----------------
Please retain the indication that these tests refer to "pr43166". (Such test files are often named pr43166.ll). 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67074