[PATCH] D50579: [LV] Teach about non header phis that have uses outside the loop

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:610
+          // if-convert the phi. May not be needed.
+          if (Phi->getNumIncomingValues() == 2 && Phi->hasNUses(1)) {
+             AllowedExit.insert(Phi);
----------------
Ayal wrote:
> Only 2 incoming values: only header phi's are required to have two incoming values; non-header phi's get if-converted for any number of predecessors.
> 
> Only 1 use: in general, it shouldn't really matter how many uses there are, nor whether I is a PHINode; if I is defined **inside** the loop, and the loop gets vectorized, it should be possible to obtain the value of I associated with the last iteration, either inside or outside the loop. Regarding cyclic dependencies, these are taken care of by analyzing each header phi, and they in turn take care of their own live-outs, by pre-computing (Inductions) or post-processing (Reductions); so any cross-iteration dependence issues will be detected, and need to be addressed, there (cf. D22341). 
> 
> Note that some Instructions may require special attention when obtaining last-lane value, e.g., `isUniformAfterVectorization()`. So starting with non-header PHI's sounds like a good first step.
I'll update the code. For now, I'm going to start with non-header PHIs as the instruction defined inside the loop and having outside use.
The "single use" check was a very conservative way to avoid cyclic dependency -will check how to address this separately during the header phi analysis for reductions/inductions/first order recurrences.


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3723
     if (LCSSAPhi.getNumIncomingValues() == 1) {
-      assert(OrigLoop->isLoopInvariant(LCSSAPhi.getIncomingValue(0)) &&
-             "Incoming value isn't loop invariant");
-      LCSSAPhi.addIncoming(LCSSAPhi.getIncomingValue(0), LoopMiddleBlock);
+      auto *IncomingValue = LCSSAPhi.getIncomingValue(0);
+      if (!Legal->isAllowedNonHeaderExit(IncomingValue)) {
----------------
Ayal wrote:
> Would it suffice to do here
> ```
>       Builder.SetInsertPoint(LoopMiddleBlock->getTerminator());
>       Value *LastIncomingValue = getOrCreateScalarValue(IncomingValue, {UF-1, VF-1});
>       LCSSAPhi.addIncoming(LastIncomingValue, LoopMiddleBlock);
> ```
> instead of the below? Note that in general, if `IncomingValue` `isUniformAfterVectorization()`, we'll need to ask for lane 0 instead of VF-1.
>From the code in `getOrCreateScalarValue`, it looks like it handles scalarization versus "unroll without vectorization" versus "unrolled and vectorized".
So, I think what you've written above is a succinct and complete version for what I need, without having to repeat the code down below. Thanks!



================
Comment at: test/Transforms/LoopVectorize/no_outside_user.ll:19
 ; CHECK-LABEL: @main(
-; CHECK-NOT: <2 x i32>
+; CHECK: <2 x i32>
 
----------------
Ayal wrote:
> Good to also check that the live-out is extracted correctly.
yes.


================
Comment at: test/Transforms/LoopVectorize/no_outside_user.ll:47
+; CHECK-LABEL: @test2(
+; CHECK: <2 x i32>
+define i32 @test2()  {
----------------
Ayal wrote:
> This vectorizes just fine, right?
vectorizes only with the patch.

Without the patch, Vectorization fails with:
```
LV: Found an outside user for :   %.lcssa = phi i32 [ %tmp17, %bb16 ]
remark: <unknown>:0:0: loop not vectorized: value could not be identified as an induction or reduction variable
LV: Can't vectorize the instructions or CFG
LV: Not vectorizing: Cannot prove legality.
```


Repository:
  rL LLVM

https://reviews.llvm.org/D50579