[PATCH] D69228: [LV] Generalize conditions for sinking instrs for first order recurrences.

[Ayal Zaks via Phabricator via llvm-commits]

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

This looks good to me, plus some final minor comments, thanks!



================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:718
+    // If the user of the PHI is also the incoming value, we potentially have a
+    // reduction and there's nothing to sink.
+    if (Previous == I)
----------------
fhahn wrote:
> Ayal wrote:
> > That's true, but it isn't checked below if Phi has more than one use, and it isn't checked in canSink (if any of I's uses is equal to SinkPoint). Perhaps it should be an assert, in all these places, verifying the precondition that Phi does not belong to any reduction cycle.
> Agreed. I guess to be sure we would have to recursively check all users, until we either reach a cycle or are outside the loop, right? I've added the (simple) check here because this is very easy to run into in practice. Would it be OK to have the assertion as follow-up?
My bad, the added check is correct; cannot_sink_reduction() test below demonstrates how this can now happen. (It could not happen until now because a cast must change types :-). And dominates(DominatedBy, U) fails if DominatedBy==U, so the users of Phi below are covered.

"and there's nothing to sink" >> "which cannot be handled by sinking" (an instruction cannot sink after itself)


================
Comment at: llvm/lib/Analysis/IVDescriptors.cpp:728
+
+    if (I->getParent() == Phi->getParent() && canSinkAfter) {
+      SinkAfter[I] = Previous;
----------------
It may be better to check allUsesDominatedBy() last, i.e., after checking for same Parents, in terms of compile-time.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll:6
+
+define void @can_sink_after_store(i32 %x, i32* %ptr, i64 %tc) local_unnamed_addr #0 {
+; CHECK-LABEL: vector.ph:
----------------
Worth commenting that the test is derived from PR43398, along with a simple version in C.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll:62
+; We can sink potential trapping instructions, as this will only delay the trap
+; and not introduce traps on additional paths.
+define void @sink_sdiv(i32 %x, i32* %ptr, i64 %tc) local_unnamed_addr #0 {
----------------
Note that a trapping instruction may sink past a trapping instruction; but if loop is vectorized, the order may swap anyhow...


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll:121
+define void @cannot_sink_with_additional_user(i32 %x, i32* %ptr, i64 %tc) {
+; CHECK-LABEL: define void @cannot_sink_with_additional_user(
+; CHECK-NEXT: entry:
----------------
Alternatively CHECK-NOT that no vector type got generated? Holds for additional not-to-be-vectorized tests below.


================
Comment at: llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll:162
+; FIXME: We can sink a store, if we can guarantee that it does no alias any
+;        loads/stores in between.
+define void @cannot_sink_store(i32 %x, i32* %ptr, i64 %tc) {
----------------
no[t] alias


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69228