Skip to content

Commit fc09adb

Browse files
[Inline][Cloning] Defer constant propagation after phi-nodes resolution
A logic issue arose when inlining via `CloneAndPruneFunctionInto`, which, besides cloning, performs basic constant propagation as well. By the time a new cloned instruction is being simplified, phi-nodes are not remapped yet as the whole CFG needs to be processed first. As `VMap` state at this stage is incomplete, `threadCmpOverPHI` and similar could lead to unsound optimizations. This issue has been addressed by postponing simplification once phi-nodes are revisited. Fixes: #87534.
1 parent 0379f2f commit fc09adb

File tree

6 files changed

+46
-70
lines changed

6 files changed

+46
-70
lines changed

llvm/lib/Transforms/Utils/CloneFunction.cpp

Lines changed: 39 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -536,29 +536,10 @@ void PruningFunctionCloner::CloneBlock(
536536
// Eagerly remap operands to the newly cloned instruction, except for PHI
537537
// nodes for which we defer processing until we update the CFG. Also defer
538538
// debug intrinsic processing because they may contain use-before-defs.
539-
if (!isa<PHINode>(NewInst) && !isa<DbgVariableIntrinsic>(NewInst)) {
539+
if (!isa<PHINode>(NewInst) && !isa<DbgVariableIntrinsic>(NewInst))
540540
RemapInstruction(NewInst, VMap,
541541
ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);
542542

543-
// If we can simplify this instruction to some other value, simply add
544-
// a mapping to that value rather than inserting a new instruction into
545-
// the basic block.
546-
if (Value *V =
547-
simplifyInstruction(NewInst, BB->getModule()->getDataLayout())) {
548-
// On the off-chance that this simplifies to an instruction in the old
549-
// function, map it back into the new function.
550-
if (NewFunc != OldFunc)
551-
if (Value *MappedV = VMap.lookup(V))
552-
V = MappedV;
553-
554-
if (!NewInst->mayHaveSideEffects()) {
555-
VMap[&*II] = V;
556-
NewInst->eraseFromParent();
557-
continue;
558-
}
559-
}
560-
}
561-
562543
if (II->hasName())
563544
NewInst->setName(II->getName() + NameSuffix);
564545
VMap[&*II] = NewInst; // Add instruction map to value.
@@ -823,52 +804,45 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
823804
}
824805
}
825806

826-
// Make a second pass over the PHINodes now that all of them have been
827-
// remapped into the new function, simplifying the PHINode and performing any
828-
// recursive simplifications exposed. This will transparently update the
829-
// WeakTrackingVH in the VMap. Notably, we rely on that so that if we coalesce
830-
// two PHINodes, the iteration over the old PHIs remains valid, and the
831-
// mapping will just map us to the new node (which may not even be a PHI
832-
// node).
833-
const DataLayout &DL = NewFunc->getParent()->getDataLayout();
834-
SmallSetVector<const Value *, 8> Worklist;
835-
for (unsigned Idx = 0, Size = PHIToResolve.size(); Idx != Size; ++Idx)
836-
if (isa<PHINode>(VMap[PHIToResolve[Idx]]))
837-
Worklist.insert(PHIToResolve[Idx]);
838-
839-
// Note that we must test the size on each iteration, the worklist can grow.
840-
for (unsigned Idx = 0; Idx != Worklist.size(); ++Idx) {
841-
const Value *OrigV = Worklist[Idx];
842-
auto *I = dyn_cast_or_null<Instruction>(VMap.lookup(OrigV));
843-
if (!I)
844-
continue;
845-
846-
// Skip over non-intrinsic callsites, we don't want to remove any nodes from
847-
// the CGSCC.
848-
CallBase *CB = dyn_cast<CallBase>(I);
849-
if (CB && CB->getCalledFunction() &&
850-
!CB->getCalledFunction()->isIntrinsic())
851-
continue;
852-
853-
// See if this instruction simplifies.
854-
Value *SimpleV = simplifyInstruction(I, DL);
855-
if (!SimpleV)
856-
continue;
857-
858-
// Stash away all the uses of the old instruction so we can check them for
859-
// recursive simplifications after a RAUW. This is cheaper than checking all
860-
// uses of To on the recursive step in most cases.
861-
for (const User *U : OrigV->users())
862-
Worklist.insert(cast<Instruction>(U));
863-
864-
// Replace the instruction with its simplified value.
865-
I->replaceAllUsesWith(SimpleV);
807+
auto GetOriginalValueInVMap = [&](const auto &I) -> Value * {
808+
for (const auto &[OrigI, NewI] : VMap) {
809+
if (NewI == I)
810+
return const_cast<Value *>(OrigI);
811+
}
812+
return nullptr;
813+
};
866814

867-
// If the original instruction had no side effects, remove it.
868-
if (isInstructionTriviallyDead(I))
869-
I->eraseFromParent();
870-
else
871-
VMap[OrigV] = I;
815+
// As phi-nodes have been now remapped, allow incremental simplification of
816+
// newly-cloned instructions.
817+
const DataLayout &DL = NewFunc->getParent()->getDataLayout();
818+
auto *NewEntryBlock = cast<BasicBlock>(VMap[StartingBB]);
819+
ReversePostOrderTraversal<BasicBlock *> RPOT(NewEntryBlock);
820+
for (auto &BB : RPOT) {
821+
for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
822+
auto *NewI = &*II++;
823+
if (isa<DbgInfoIntrinsic>(NewI))
824+
continue;
825+
826+
CallBase *CB = dyn_cast<CallBase>(NewI);
827+
if (CB && CB->getCalledFunction() &&
828+
!CB->getCalledFunction()->isIntrinsic())
829+
continue;
830+
831+
if (Value *V = simplifyInstruction(NewI, DL)) {
832+
if (!NewI->use_empty())
833+
NewI->replaceAllUsesWith(V);
834+
835+
if (isInstructionTriviallyDead(NewI)) {
836+
NewI->eraseFromParent();
837+
} else {
838+
// Did not erase it? Restore the new instruction into VMap.
839+
auto *OriginalI = GetOriginalValueInVMap(NewI);
840+
assert(OriginalI != nullptr &&
841+
"Previously remapped value was not found?");
842+
VMap[OriginalI] = NewI;
843+
}
844+
}
845+
}
872846
}
873847

874848
// Remap debug intrinsic operands now that all values have been mapped.

llvm/test/Transforms/Inline/dynamic-alloca-simplified-large.ll

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,8 @@ define void @caller3_alloca_not_in_entry(ptr %p1, i1 %b) {
107107
; CHECK-NEXT: [[COND:%.*]] = icmp eq i1 [[B]], true
108108
; CHECK-NEXT: br i1 [[COND]], label [[EXIT:%.*]], label [[SPLIT:%.*]]
109109
; CHECK: split:
110+
; CHECK-NEXT: [[SAVEDSTACK:%.*]] = call ptr @llvm.stacksave.p0()
111+
; CHECK-NEXT: call void @llvm.stackrestore.p0(ptr [[SAVEDSTACK]])
110112
; CHECK-NEXT: br label [[EXIT]]
111113
; CHECK: exit:
112114
; CHECK-NEXT: ret void

llvm/test/Transforms/Inline/inline-deferred-instsimplify.ll

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,8 +38,6 @@ store_ptr_in_gvar: ; preds = %entry
3838

3939
check_pointers_are_equal: ; preds = %store_ptr_in_gvar, %entry
4040
%phi = phi ptr [ %ptr, %store_ptr_in_gvar ], [ @other_g_var, %entry ]
41-
; FIXME: While inlining, the following is miscompiled to i1 false,
42-
; as %ptr in the phi-node is not taken into account.
4341
%.not1 = icmp eq ptr %phi, %ptr
4442
br i1 %.not1, label %return, label %abort
4543

@@ -64,9 +62,13 @@ define i32 @main() {
6462
; CHECK-NEXT: br label [[CHECK_POINTERS_ARE_EQUAL_I]]
6563
; CHECK: check_pointers_are_equal.i:
6664
; CHECK-NEXT: [[PHI_I:%.*]] = phi ptr [ [[G_VAR]], [[STORE_PTR_IN_GVAR_I]] ], [ @other_g_var, [[TMP0:%.*]] ]
65+
; CHECK-NEXT: [[DOTNOT1_I:%.*]] = icmp eq ptr [[PHI_I]], [[G_VAR]]
66+
; CHECK-NEXT: br i1 [[DOTNOT1_I]], label [[CALLEE_EXIT:%.*]], label [[ABORT_I:%.*]]
67+
; CHECK: abort.i:
6768
; CHECK-NEXT: call void @abort()
6869
; CHECK-NEXT: unreachable
6970
; CHECK: callee.exit:
71+
; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 20, ptr [[G_VAR]])
7072
; CHECK-NEXT: ret i32 0
7173
;
7274
call void @callee(ptr noundef byval(%struct.a) align 8 @g_var)

llvm/test/Transforms/Inline/prof-update-instr.ll

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,6 @@ define void @caller() !prof !21 {
5252
!21 = !{!"function_entry_count", i64 400}
5353
attributes #0 = { alwaysinline }
5454
; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
55-
; CHECK: ![[COUNT_IND_CALLEE1]] = !{!"VP", i32 0, i64 200, i64 111, i64 100, i64 222, i64 60, i64 333, i64 40}
55+
; CHECK: ![[COUNT_IND_CALLEE1]] = !{!"VP", i32 0, i64 120, i64 111, i64 60, i64 222, i64 36, i64 333, i64 24}
5656
; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
5757
; CHECK: ![[COUNT_IND_CALLER]] = !{!"VP", i32 0, i64 56, i64 111, i64 32, i64 222, i64 16, i64 333, i64 8}

llvm/test/Transforms/Inline/prof-update-sample-alwaysinline.ll

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,6 @@ define void @caller() {
5353
!18 = !{!"VP", i32 0, i64 140, i64 111, i64 80, i64 222, i64 40, i64 333, i64 20}
5454
attributes #0 = { alwaysinline }
5555
; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
56-
; CHECK: ![[COUNT_CALLEE1]] = !{!"branch_weights", i32 2000}
5756
; CHECK: ![[COUNT_CALLEE]] = !{!"branch_weights", i32 1200}
5857
; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
5958
; CHECK: ![[COUNT_CALLER]] = !{!"branch_weights", i32 800}

llvm/test/Transforms/Inline/prof-update-sample.ll

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ define void @caller() {
5252
!17 = !{!"branch_weights", i32 400}
5353
!18 = !{!"VP", i32 0, i64 140, i64 111, i64 80, i64 222, i64 40, i64 333, i64 20}
5454
; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
55-
; CHECK: ![[COUNT_CALLEE1]] = !{!"branch_weights", i32 2000}
5655
; CHECK: ![[COUNT_CALLEE]] = !{!"branch_weights", i32 1200}
5756
; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
5857
; CHECK: ![[COUNT_CALLER]] = !{!"branch_weights", i32 800}

0 commit comments

Comments
 (0)