[PATCH] D50778: [LV] Vectorize loops where non-phi instructions used outside loop

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.
Herald added a subscriber: rkruppe.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:582
 
+  bool PredicateValidOutsideLoop = PSE.getUnionPredicate().isAlwaysTrue();
+
----------------
Ayal wrote:
> Use `PredicateValidOutsideLoop` below?
that doesn't work. I'd started off with it and the pr33706.ll test case failed. Specifically, the predicates were added as part of `IsInductionPhi` within the loop over instructions below. So, we need to check for the predicates at the time of adding valid exits that can be used outside the loop.

I'll remove the dead variable declaration here.


================
Comment at: lib/Transforms/Vectorize/LoopVectorizationLegality.cpp:728
+          continue;
+        }
         ORE->emit(createMissedAnalysis("ValueUsedOutsideLoop", &I)
----------------
Ayal wrote:
> It would now be good to reason about and mark all exits that are **not allowed** to be live-out, rather than those that are :-). Specifically, Reduction Phi's are on this "not allowed" list, as they represent the one-before-last value, which is not available when vectorized (could possibly be computed by "subtracting" from the final reduced value), unlike the Reduction bump instructions. Induction Phi's and their bumps are, as pointed out here, also on this "not allowed" list when the SCEV predicates cannot be used outside the loop, because the latter are used to pre-compute the live-out values. But such cases could possibly be handled alternatively by extracting from within the loop, similar to internal Instructions, rather than pre-computing. FirstOrderRecurrence Phi's are also currently one the "not allowed" list, unlike their "Previous" value, and could also possibly be handled using extraction. Would be good to confirm that this enumeration is exhaustive.
I prefer this being a follow-on NFC. By having it as a follow-on "intended" NFC, it also confirms the enumeration for the "not allowed list" is exhaustive :)




Repository:
  rL LLVM

https://reviews.llvm.org/D50778