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

Sat Dec 7 11:44:05 PST 2019

fhahn marked 3 inline comments as done.
fhahn added a comment.

In D71071#1772329 <https://reviews.llvm.org/D71071#1772329>, @Ayal wrote:

> 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.

I tried, but the test case requires tail folding and it seems to depend on an actual target. We make the decision to tail-fold in computeMaxVF (https://github.com/llvm/llvm-project/blob/c49194969430f0ee817498a7000a979a7a0ded03/llvm/lib/Transforms/Vectorize/LoopVectorize.cpp#L4941) and use the max target VF, not the forced vector width. This seems a bit odd and we might want to change that, to make it easier to test tail-folding without any specific target. What do you think?

================
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());
----------------
Ayal wrote:
> (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?)
I've added a FIXME in the committed version

================
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
----------------
Ayal wrote:
> Ayal wrote:
> > 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." ;-)
> Mention PR44020
Done thanks :) 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71071