-
Notifications
You must be signed in to change notification settings - Fork 15.3k
[InstCombine] Push freeze through non-recurrence PHIs #157678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
03e6ac1
3088389
374eac2
3a17a82
04b130d
5813f51
8481016
e037b3e
840c626
c308125
baa9495
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5027,10 +5027,33 @@ InstCombinerImpl::pushFreezeToPreventPoisonFromPropagating(FreezeInst &OrigFI) { | |
| // Op1.fr = Freeze(Op1) | ||
| // ... = Inst(Op1.fr, NonPoisonOps...) | ||
|
|
||
| auto CanPushFreeze = [](Value *V) { | ||
| if (!isa<Instruction>(V) || isa<PHINode>(V)) | ||
| auto CanPushFreeze = [this](Value *V) { | ||
| if (!isa<Instruction>(V)) | ||
| return false; | ||
|
|
||
| if (auto *PN = dyn_cast<PHINode>(V)) { | ||
| BasicBlock *BB = PN->getParent(); | ||
| SmallPtrSet<BasicBlock *, 8> VisitedBBs; | ||
| for (Use &U : PN->incoming_values()) { | ||
| BasicBlock *InBB = PN->getIncomingBlock(U); | ||
| // We can't move freeze if the start value is the result of a | ||
| // terminator (e.g. an invoke). | ||
| if (auto *OpI = dyn_cast<Instruction>(U)) { | ||
| if (OpI->isTerminator()) | ||
| return false; | ||
| } | ||
|
|
||
| // If there's multiple incoming edges from the same predecessor we must | ||
| // ensure the freeze isn't pushed to a single one of the uses, | ||
| // invalidating the iterator. We simply don't support this case, but it | ||
| // could be handled if there's a use case. | ||
| if (isBackEdge(InBB, BB) || !VisitedBBs.insert(InBB).second || | ||
| match(U.get(), m_Undef())) | ||
| return false; | ||
| VisitedBBs.insert(InBB); | ||
| } | ||
| } | ||
|
|
||
| // We can't push the freeze through an instruction which can itself create | ||
| // poison. If the only source of new poison is flags, we can simply | ||
| // strip them (since we know the only use is the freeze and nothing can | ||
|
|
@@ -5049,13 +5072,25 @@ InstCombinerImpl::pushFreezeToPreventPoisonFromPropagating(FreezeInst &OrigFI) { | |
| while (!Worklist.empty()) { | ||
| auto *U = Worklist.pop_back_val(); | ||
| Value *V = U->get(); | ||
|
|
||
| if (auto *PN = dyn_cast<PHINode>(V)) { | ||
| if (FreezePhisVisited.contains(PN)) { | ||
| LLVM_DEBUG(dbgs() << "freeze has already been pushed through PHI '" | ||
| << PN->getName() << "', skipping.\n"); | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if (!CanPushFreeze(V)) { | ||
| // If we can't push through the original instruction, abort the transform. | ||
| if (U == OrigUse) | ||
| return nullptr; | ||
|
|
||
| auto *UserI = cast<Instruction>(U->getUser()); | ||
| Builder.SetInsertPoint(UserI); | ||
| if (auto *PN = dyn_cast<PHINode>(UserI)) | ||
| Builder.SetInsertPoint(PN->getIncomingBlock(*U)->getTerminator()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not quite sure why this change is needed. Before this PR Is this fixing an existing bug?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the existing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps I'm missing something, but before your patch CanPushFreeze always returned false for PHI nodes which meant we were calling However, even in places where we still return false for PHI nodes same as before we're now calling
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that CanPushFreeze() is called on the use, while this is checking for the user being a phi node. Previously we could not have ended up with a phi user, as that would imply that we have pushed through a phi.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok I think I understand now. So basically if CanPushFreeze returns true for a PHI node we fall through to code below that adds the operands of the PHI to the worklist. Then if CanPushFreeze returns false for one of those operands we'll end up asking for the user, which will be the PHI. |
||
| else | ||
| Builder.SetInsertPoint(UserI); | ||
c-rhodes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Value *Frozen = Builder.CreateFreeze(V, V->getName() + ".fr"); | ||
| U->set(Frozen); | ||
| continue; | ||
|
|
@@ -5075,6 +5110,8 @@ InstCombinerImpl::pushFreezeToPreventPoisonFromPropagating(FreezeInst &OrigFI) { | |
|
|
||
| I->dropPoisonGeneratingAnnotations(); | ||
| this->Worklist.add(I); | ||
| if (auto *PN = dyn_cast<PHINode>(V)) | ||
| FreezePhisVisited.insert(PN); | ||
| } | ||
|
|
||
| return OrigUse->get(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| ; RUN: opt < %s -passes=instcombine -S -debug 2>&1 | FileCheck %s | ||
| ; REQUIRES: asserts | ||
|
|
||
| ; Repeatedly pushing freeze thru PHIs can be expensive so we keep track of PHIs | ||
| ; for which freeze has already been pushed thru. | ||
|
|
||
| define i1 @a(i1 %min.iters.check2957, <4 x double> %wide.load2970, <4 x i1> %0, <4 x i1> %1) { | ||
| .lr.ph1566: | ||
| br i1 %min.iters.check2957, label %vec.epilog.ph2984, label %vector.ph2959 | ||
|
|
||
| vector.ph2959: ; preds = %.lr.ph1566 | ||
| %2 = fcmp olt <4 x double> %wide.load2970, zeroinitializer | ||
| %bin.rdx2976 = or <4 x i1> %2, %0 | ||
| %bin.rdx2977 = or <4 x i1> %1, %bin.rdx2976 | ||
| %3 = call i1 @llvm.vector.reduce.or.v4i1(<4 x i1> %bin.rdx2977) | ||
| %rdx.select2979 = select i1 %3, i32 1, i32 0 | ||
| br label %vec.epilog.ph2984 | ||
|
|
||
| vec.epilog.ph2984: ; preds = %vector.ph2959, %.lr.ph1566 | ||
| %bc.merge.rdx2982 = phi i32 [ %rdx.select2979, %vector.ph2959 ], [ 0, %.lr.ph1566 ] | ||
| %4 = icmp ne i32 %bc.merge.rdx2982, 0 | ||
| %broadcast.splatinsert2992 = insertelement <2 x i1> zeroinitializer, i1 %4, i64 0 | ||
| %5 = call i1 @llvm.vector.reduce.or.v2i1(<2 x i1> %broadcast.splatinsert2992) | ||
| %6 = freeze i1 %5 | ||
| %7 = freeze i1 %6 | ||
| ret i1 %7 | ||
| } | ||
|
|
||
| ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none) | ||
| declare i1 @llvm.vector.reduce.or.v4i1(<4 x i1>) #0 | ||
|
|
||
| ; Function Attrs: nocallback nofree nosync nounwind speculatable willreturn memory(none) | ||
| declare i1 @llvm.vector.reduce.or.v2i1(<2 x i1>) #0 | ||
|
|
||
| attributes #0 = { nocallback nofree nosync nounwind speculatable willreturn memory(none) } | ||
|
|
||
| ; CHECK: freeze has already been pushed through PHI 'bc.merge.rdx2982', skipping. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've simplified the change to introduce a cache as @paulwalker-arm suggested to restrict it to
pushFreezeToPreventPoisonFromPropagating. It also now assumes it's ok to push a freeze thru the PHI if it's been done before. To be honest I'm not entirely sure if the cache could be invalidated somehow making this unsafe, @nikic would appreciate your expertise there. If so, the original cache I added shouldnt have this problem at least, or perhaps you have a better idea.