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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Overall looks good to me, though it could be cleaned up a bit more?



================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:438
   // Reduction and Induction instructions are allowed to have exit users. All
   // other instructions must not have external users.
   if (!AllowedExit.count(Inst))
----------------
Worth updating the above comment, to also include non-header Phi's.

Perhaps leave behind a `TODO` somewhere, that other (non-Phi) instructions should also be able to work with the new extract-scalar-of-last-iteration code.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:602
           // identified reduction value with an outside user.
           if (!hasOutsideLoopUser(TheLoop, Phi, AllowedExit))
             continue;
----------------
Checking if `Phi` has outside users here becomes redundant, when `AllowNonHdrExits` becomes redundant (see below).


================
Comment at: lib/Transforms/Vectorize/LoopVectorize.cpp:3724
+      auto *IncomingValue = LCSSAPhi.getIncomingValue(0);
+      if (!Legal->isAllowedNonHeaderExit(IncomingValue)) {
+        assert(OrigLoop->isLoopInvariant(LCSSAPhi.getIncomingValue(0)) &&
----------------
Distinguishing between different `AllowedExits` here seem redundant: if `IncomingValue` `isLoopInvariant()`, as asserted below, `getOrCreateScalarValue({UF-1,VF-1})` will simply return it. So the 'else' case below can also take care of the 'then' case. In general, whatever the allowed exit may be, grab the scalar value associated with the last iteration.

This will make `AllowedNonHdrExits` useless.

In the most general form, one may need to do
```
  unsigned LastLane = Cost->isUniformAfterVectorization(IncomingValue, VF) ? 0 : VF - 1;
  Value *lastIncomingValue =
            getOrCreateScalarValue(IncomingValue, {UF - 1, LastLane});
```
but this is not needed for this patch.


================
Comment at: test/Transforms/LoopVectorize/no_outside_user.ll:12-16
+; It has a value that is used outside of the loop
 ; and is not a recognized reduction variable "tmp17".
+; However, tmp17 does not have a cyclic dependence with any header phis. So, the
+; iteration dependence distance for tmp17 can also be widened to VF (just like
+; we do for reduction/induction).
----------------
Suffice to say "tmp17" is a non-header phi, which is an allowed exit.


================
Comment at: test/Transforms/LoopVectorize/no_outside_user.ll:92
+; CHECK:          %predphi = select <2 x i1>
+; CHECK:          %predphi1 = select <2 x i1>
+
----------------
(Check that) `%predphi1` has `%predphi` as one of its operands, right? Thereby merging the more than 2 original incoming values together.


================
Comment at: test/Transforms/LoopVectorize/no_outside_user.ll:207
+; cyclic dependency with induction var
+; cannot vectorize.
+; CHECK-LABEL: cyclic_dep_with_indvar(
----------------
In `cyclic_dep_with_indvar()` below, `%iv` is conditionally reset inside the loop, disqualifying it from being an induction variable; so the loop isn't vectorizable, regardless of live-outs. Right? 


Repository:
  rL LLVM

https://reviews.llvm.org/D50579