Skip to content

Commit eaff300

Browse files
committed
Revert f0c2a5a "[LV] Generalize conditions for sinking instrs for first order recurrences."
It broke Chromium, causing "Instruction does not dominate all uses!" errors. See https://bugs.chromium.org/p/chromium/issues/detail?id=1022297#c1 for a reproducer. > If the recurrence PHI node has a single user, we can sink any > instruction without side effects, given that all users are dominated by > the instruction computing the incoming value of the next iteration > ('Previous'). We can sink instructions that may cause traps, because > that only causes the trap to occur later, but not on any new paths. > > With the relaxed check, we also have to make sure that we do not have a > direct cycle (meaning PHI user == 'Previous), which indicates a > reduction relation, which potentially gets missed by > ReductionDescriptor. > > As follow-ups, we can also sink stores, iff they do not alias with > other instructions we move them across and we could also support sinking > chains of instructions and multiple users of the PHI. > > Fixes PR43398. > > Reviewers: hsaito, dcaballe, Ayal, rengolin > > Reviewed By: Ayal > > Differential Revision: https://reviews.llvm.org/D69228
1 parent c5e4cf4 commit eaff300

File tree

2 files changed

+14
-271
lines changed

2 files changed

+14
-271
lines changed

llvm/lib/Analysis/IVDescriptors.cpp

Lines changed: 14 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -699,37 +699,25 @@ bool RecurrenceDescriptor::isFirstOrderRecurrence(
699699
// Ensure every user of the phi node is dominated by the previous value.
700700
// The dominance requirement ensures the loop vectorizer will not need to
701701
// vectorize the initial value prior to the first iteration of the loop.
702-
// TODO: Consider extending this sinking to handle memory instructions and
703-
// phis with multiple users.
704-
705-
// Returns true, if all users of I are dominated by DominatedBy.
706-
auto allUsesDominatedBy = [DT](Instruction *I, Instruction *DominatedBy) {
707-
return all_of(I->uses(), [DT, DominatedBy](Use &U) {
708-
return DT->dominates(DominatedBy, U);
709-
});
710-
};
711-
702+
// TODO: Consider extending this sinking to handle other kinds of instructions
703+
// and expressions, beyond sinking a single cast past Previous.
712704
if (Phi->hasOneUse()) {
713-
Instruction *I = Phi->user_back();
714-
715-
// If the user of the PHI is also the incoming value, we potentially have a
716-
// reduction and which cannot be handled by sinking.
717-
if (Previous == I)
718-
return false;
719-
720-
if (DT->dominates(Previous, I)) // We already are good w/o sinking.
721-
return true;
722-
723-
// We can sink any instruction without side effects, as long as all users
724-
// are dominated by the instruction we are sinking after.
725-
if (I->getParent() == Phi->getParent() && !I->mayHaveSideEffects() &&
726-
allUsesDominatedBy(I, Previous)) {
727-
SinkAfter[I] = Previous;
705+
auto *I = Phi->user_back();
706+
if (I->isCast() && (I->getParent() == Phi->getParent()) && I->hasOneUse() &&
707+
DT->dominates(Previous, I->user_back())) {
708+
if (!DT->dominates(Previous, I)) // Otherwise we're good w/o sinking.
709+
SinkAfter[I] = Previous;
728710
return true;
729711
}
730712
}
731713

732-
return allUsesDominatedBy(Phi, Previous);
714+
for (User *U : Phi->users())
715+
if (auto *I = dyn_cast<Instruction>(U)) {
716+
if (!DT->dominates(Previous, I))
717+
return false;
718+
}
719+
720+
return true;
733721
}
734722

735723
/// This function returns the identity element (or neutral element) for

llvm/test/Transforms/LoopVectorize/first-order-recurrence-complex.ll

Lines changed: 0 additions & 245 deletions
This file was deleted.

0 commit comments

Comments
 (0)