[PATCH] D71071: [LV] Pick correct BB as insert point when fixing PHI for FORs.

[Ayal Zaks via Phabricator via llvm-commits]

Ayal accepted this revision.
Ayal added a comment.
This revision is now accepted and ready to land.

Looks good to me with minor optional notes.
Consider adding a target-indepedent test, e.g., augmenting first-order-recurrence.ll; tests can possibly be committed before the fix, demonstrating current behavior and then the change brought by this fix.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3557
+  // guaranteed to be an instruction in the vector loop.
+  if (LI->getLoopFor(LoopVectorBody)->isLoopInvariant(PreviousLastPart))
     Builder.SetInsertPoint(&*LoopVectorBody->getFirstInsertionPt());
----------------
(Note that Previous that is loop invariant forms no recurrence ... they best be recognized early (if/why not LICM'd prior to LV), and optimized accordingly.  A todo for/following D68831?)


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:3568
     Builder.SetInsertPoint(
         &*++BasicBlock::iterator(cast<Instruction>(PreviousLastPart)));
 
----------------
Can refactor to have a single `Builder.SetInsertPoint(Point);` following the different cases for setting Point.


================
Comment at: llvm/test/Transforms/LoopVectorize/SystemZ/predicated-first-order-recurrence.ll:3
+; RUN: opt -loop-vectorize -mtriple=s390x-ibm-linux -mcpu=z13 -force-vector-width=2 -S %s | FileCheck %s
+
+ at A = external dso_local global [5 x i32], align 4
----------------
State what this test is designed to check?
("In the test case we predicate due to low trip count right? It might be worth calling that out." ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71071