[Inline][Cloning] Defer constant propagation after phi-nodes resolution #87963
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: Antonio Frighetto (antoniofrighetto) ChangesA logic issue arose when inlining via Fixes: #87534. Full diff: https://github.com/llvm/llvm-project/pull/87963.diff 6 Files Affected:
diff --git a/llvm/lib/Transforms/Utils/CloneFunction.cpp b/llvm/lib/Transforms/Utils/CloneFunction.cpp
index 3eac726994ae13..627174d1b354e4 100644
--- a/llvm/lib/Transforms/Utils/CloneFunction.cpp
+++ b/llvm/lib/Transforms/Utils/CloneFunction.cpp
@@ -536,29 +536,10 @@ void PruningFunctionCloner::CloneBlock(
// Eagerly remap operands to the newly cloned instruction, except for PHI
// nodes for which we defer processing until we update the CFG. Also defer
// debug intrinsic processing because they may contain use-before-defs.
- if (!isa<PHINode>(NewInst) && !isa<DbgVariableIntrinsic>(NewInst)) {
+ if (!isa<PHINode>(NewInst) && !isa<DbgVariableIntrinsic>(NewInst))
RemapInstruction(NewInst, VMap,
ModuleLevelChanges ? RF_None : RF_NoModuleLevelChanges);
- // If we can simplify this instruction to some other value, simply add
- // a mapping to that value rather than inserting a new instruction into
- // the basic block.
- if (Value *V =
- simplifyInstruction(NewInst, BB->getModule()->getDataLayout())) {
- // On the off-chance that this simplifies to an instruction in the old
- // function, map it back into the new function.
- if (NewFunc != OldFunc)
- if (Value *MappedV = VMap.lookup(V))
- V = MappedV;
-
- if (!NewInst->mayHaveSideEffects()) {
- VMap[&*II] = V;
- NewInst->eraseFromParent();
- continue;
- }
- }
- }
-
if (II->hasName())
NewInst->setName(II->getName() + NameSuffix);
VMap[&*II] = NewInst; // Add instruction map to value.
@@ -823,52 +804,45 @@ void llvm::CloneAndPruneIntoFromInst(Function *NewFunc, const Function *OldFunc,
}
}
- // Make a second pass over the PHINodes now that all of them have been
- // remapped into the new function, simplifying the PHINode and performing any
- // recursive simplifications exposed. This will transparently update the
- // WeakTrackingVH in the VMap. Notably, we rely on that so that if we coalesce
- // two PHINodes, the iteration over the old PHIs remains valid, and the
- // mapping will just map us to the new node (which may not even be a PHI
- // node).
- const DataLayout &DL = NewFunc->getParent()->getDataLayout();
- SmallSetVector<const Value *, 8> Worklist;
- for (unsigned Idx = 0, Size = PHIToResolve.size(); Idx != Size; ++Idx)
- if (isa<PHINode>(VMap[PHIToResolve[Idx]]))
- Worklist.insert(PHIToResolve[Idx]);
-
- // Note that we must test the size on each iteration, the worklist can grow.
- for (unsigned Idx = 0; Idx != Worklist.size(); ++Idx) {
- const Value *OrigV = Worklist[Idx];
- auto *I = dyn_cast_or_null<Instruction>(VMap.lookup(OrigV));
- if (!I)
- continue;
-
- // Skip over non-intrinsic callsites, we don't want to remove any nodes from
- // the CGSCC.
- CallBase *CB = dyn_cast<CallBase>(I);
- if (CB && CB->getCalledFunction() &&
- !CB->getCalledFunction()->isIntrinsic())
- continue;
-
- // See if this instruction simplifies.
- Value *SimpleV = simplifyInstruction(I, DL);
- if (!SimpleV)
- continue;
-
- // Stash away all the uses of the old instruction so we can check them for
- // recursive simplifications after a RAUW. This is cheaper than checking all
- // uses of To on the recursive step in most cases.
- for (const User *U : OrigV->users())
- Worklist.insert(cast<Instruction>(U));
-
- // Replace the instruction with its simplified value.
- I->replaceAllUsesWith(SimpleV);
+ auto GetOriginalValueInVMap = [&](const auto &I) -> Value * {
+ for (const auto &[OrigI, NewI] : VMap) {
+ if (NewI == I)
+ return const_cast<Value *>(OrigI);
+ }
+ return nullptr;
+ };
- // If the original instruction had no side effects, remove it.
- if (isInstructionTriviallyDead(I))
- I->eraseFromParent();
- else
- VMap[OrigV] = I;
+ // As phi-nodes have been now remapped, allow incremental simplification of
+ // newly-cloned instructions.
+ const DataLayout &DL = NewFunc->getParent()->getDataLayout();
+ auto *NewEntryBlock = cast<BasicBlock>(VMap[StartingBB]);
+ ReversePostOrderTraversal<BasicBlock *> RPOT(NewEntryBlock);
+ for (auto &BB : RPOT) {
+ for (BasicBlock::iterator II = BB->begin(), IE = BB->end(); II != IE;) {
+ auto *NewI = &*II++;
+ if (isa<DbgInfoIntrinsic>(NewI))
+ continue;
+
+ CallBase *CB = dyn_cast<CallBase>(NewI);
+ if (CB && CB->getCalledFunction() &&
+ !CB->getCalledFunction()->isIntrinsic())
+ continue;
+
+ if (Value *V = simplifyInstruction(NewI, DL)) {
+ if (!NewI->use_empty())
+ NewI->replaceAllUsesWith(V);
+
+ if (isInstructionTriviallyDead(NewI)) {
+ NewI->eraseFromParent();
+ } else {
+ // Did not erase it? Restore the new instruction into VMap.
+ auto *OriginalI = GetOriginalValueInVMap(NewI);
+ assert(OriginalI != nullptr &&
+ "Previously remapped value was not found?");
+ VMap[OriginalI] = NewI;
+ }
+ }
+ }
}
// Remap debug intrinsic operands now that all values have been mapped.
diff --git a/llvm/test/Transforms/Inline/dynamic-alloca-simplified-large.ll b/llvm/test/Transforms/Inline/dynamic-alloca-simplified-large.ll
index 9b293d39c85f95..54e2010f0b0e9f 100644
--- a/llvm/test/Transforms/Inline/dynamic-alloca-simplified-large.ll
+++ b/llvm/test/Transforms/Inline/dynamic-alloca-simplified-large.ll
@@ -107,6 +107,8 @@ define void @caller3_alloca_not_in_entry(ptr %p1, i1 %b) {
; CHECK-NEXT: [[COND:%.*]] = icmp eq i1 [[B]], true
; CHECK-NEXT: br i1 [[COND]], label [[EXIT:%.*]], label [[SPLIT:%.*]]
; CHECK: split:
+; CHECK-NEXT: [[SAVEDSTACK:%.*]] = call ptr @llvm.stacksave.p0()
+; CHECK-NEXT: call void @llvm.stackrestore.p0(ptr [[SAVEDSTACK]])
; CHECK-NEXT: br label [[EXIT]]
; CHECK: exit:
; CHECK-NEXT: ret void
diff --git a/llvm/test/Transforms/Inline/inline-deferred-instsimplify.ll b/llvm/test/Transforms/Inline/inline-deferred-instsimplify.ll
new file mode 100644
index 00000000000000..f02d03688f0397
--- /dev/null
+++ b/llvm/test/Transforms/Inline/inline-deferred-instsimplify.ll
@@ -0,0 +1,78 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt < %s -passes=inline -S | FileCheck %s
+; RUN: opt < %s -passes='cgscc(inline)' -S | FileCheck %s
+
+%struct.a = type { i32, i32, i32, i32, i32 }
+
+@g_var = global %struct.a { i32 1, i32 0, i32 0, i32 0, i32 0 }, align 8
+@other_g_var = global %struct.a zeroinitializer, align 4
+
+define void @callee(ptr noundef byval(%struct.a) align 8 %ptr) {
+; CHECK-LABEL: define void @callee(
+; CHECK-SAME: ptr noundef byval([[STRUCT_A:%.*]]) align 8 [[PTR:%.*]]) {
+; CHECK-NEXT: entry:
+; CHECK-NEXT: [[VAL:%.*]] = load i32, ptr [[PTR]], align 8
+; CHECK-NEXT: [[DOTNOT:%.*]] = icmp eq i32 [[VAL]], 0
+; CHECK-NEXT: br i1 [[DOTNOT]], label [[CHECK_POINTERS_ARE_EQUAL:%.*]], label [[STORE_PTR_IN_GVAR:%.*]]
+; CHECK: store_ptr_in_gvar:
+; CHECK-NEXT: store ptr [[PTR]], ptr @other_g_var, align 8
+; CHECK-NEXT: br label [[CHECK_POINTERS_ARE_EQUAL]]
+; CHECK: check_pointers_are_equal:
+; CHECK-NEXT: [[PHI:%.*]] = phi ptr [ [[PTR]], [[STORE_PTR_IN_GVAR]] ], [ @other_g_var, [[ENTRY:%.*]] ]
+; CHECK-NEXT: [[DOTNOT1:%.*]] = icmp eq ptr [[PHI]], [[PTR]]
+; CHECK-NEXT: br i1 [[DOTNOT1]], label [[RETURN:%.*]], label [[ABORT:%.*]]
+; CHECK: abort:
+; CHECK-NEXT: call void @abort()
+; CHECK-NEXT: unreachable
+; CHECK: return:
+; CHECK-NEXT: ret void
+;
+entry:
+ %val = load i32, ptr %ptr, align 8
+ %.not = icmp eq i32 %val, 0
+ br i1 %.not, label %check_pointers_are_equal, label %store_ptr_in_gvar
+
+store_ptr_in_gvar: ; preds = %entry
+ store ptr %ptr, ptr @other_g_var, align 8
+ br label %check_pointers_are_equal
+
+check_pointers_are_equal: ; preds = %store_ptr_in_gvar, %entry
+ %phi = phi ptr [ %ptr, %store_ptr_in_gvar ], [ @other_g_var, %entry ]
+ %.not1 = icmp eq ptr %phi, %ptr
+ br i1 %.not1, label %return, label %abort
+
+abort: ; preds = %check_pointers_are_equal
+ call void @abort()
+ unreachable
+
+return: ; preds = %check_pointers_are_equal
+ ret void
+}
+
+define i32 @main() {
+; CHECK-LABEL: define i32 @main() {
+; CHECK-NEXT: [[G_VAR:%.*]] = alloca [[STRUCT_A:%.*]], align 8
+; CHECK-NEXT: call void @llvm.lifetime.start.p0(i64 20, ptr [[G_VAR]])
+; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i64(ptr align 1 [[G_VAR]], ptr align 1 @g_var, i64 20, i1 false)
+; CHECK-NEXT: [[VAL_I:%.*]] = load i32, ptr [[G_VAR]], align 8
+; CHECK-NEXT: [[DOTNOT_I:%.*]] = icmp eq i32 [[VAL_I]], 0
+; CHECK-NEXT: br i1 [[DOTNOT_I]], label [[CHECK_POINTERS_ARE_EQUAL_I:%.*]], label [[STORE_PTR_IN_GVAR_I:%.*]]
+; CHECK: store_ptr_in_gvar.i:
+; CHECK-NEXT: store ptr [[G_VAR]], ptr @other_g_var, align 8
+; CHECK-NEXT: br label [[CHECK_POINTERS_ARE_EQUAL_I]]
+; CHECK: check_pointers_are_equal.i:
+; CHECK-NEXT: [[PHI_I:%.*]] = phi ptr [ [[G_VAR]], [[STORE_PTR_IN_GVAR_I]] ], [ @other_g_var, [[TMP0:%.*]] ]
+; CHECK-NEXT: [[DOTNOT1_I:%.*]] = icmp eq ptr [[PHI_I]], [[G_VAR]]
+; CHECK-NEXT: br i1 [[DOTNOT1_I]], label [[CALLEE_EXIT:%.*]], label [[ABORT_I:%.*]]
+; CHECK: abort.i:
+; CHECK-NEXT: call void @abort()
+; CHECK-NEXT: unreachable
+; CHECK: callee.exit:
+; CHECK-NEXT: call void @llvm.lifetime.end.p0(i64 20, ptr [[G_VAR]])
+; CHECK-NEXT: ret i32 0
+;
+ call void @callee(ptr noundef byval(%struct.a) align 8 @g_var)
+ ret i32 0
+}
+
+declare void @abort()
diff --git a/llvm/test/Transforms/Inline/prof-update-instr.ll b/llvm/test/Transforms/Inline/prof-update-instr.ll
index 38dfa67dcacdc5..01a515bc8e293e 100644
--- a/llvm/test/Transforms/Inline/prof-update-instr.ll
+++ b/llvm/test/Transforms/Inline/prof-update-instr.ll
@@ -52,6 +52,6 @@ define void @caller() !prof !21 {
!21 = !{!"function_entry_count", i64 400}
attributes #0 = { alwaysinline }
; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
-; CHECK: ![[COUNT_IND_CALLEE1]] = !{!"VP", i32 0, i64 200, i64 111, i64 100, i64 222, i64 60, i64 333, i64 40}
+; CHECK: ![[COUNT_IND_CALLEE1]] = !{!"VP", i32 0, i64 120, i64 111, i64 60, i64 222, i64 36, i64 333, i64 24}
; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
; CHECK: ![[COUNT_IND_CALLER]] = !{!"VP", i32 0, i64 56, i64 111, i64 32, i64 222, i64 16, i64 333, i64 8}
diff --git a/llvm/test/Transforms/Inline/prof-update-sample-alwaysinline.ll b/llvm/test/Transforms/Inline/prof-update-sample-alwaysinline.ll
index d6b771e2629d2e..8af4d89663a4da 100644
--- a/llvm/test/Transforms/Inline/prof-update-sample-alwaysinline.ll
+++ b/llvm/test/Transforms/Inline/prof-update-sample-alwaysinline.ll
@@ -53,7 +53,6 @@ define void @caller() {
!18 = !{!"VP", i32 0, i64 140, i64 111, i64 80, i64 222, i64 40, i64 333, i64 20}
attributes #0 = { alwaysinline }
; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
-; CHECK: ![[COUNT_CALLEE1]] = !{!"branch_weights", i32 2000}
; CHECK: ![[COUNT_CALLEE]] = !{!"branch_weights", i32 1200}
; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
; CHECK: ![[COUNT_CALLER]] = !{!"branch_weights", i32 800}
diff --git a/llvm/test/Transforms/Inline/prof-update-sample.ll b/llvm/test/Transforms/Inline/prof-update-sample.ll
index 6cdd70e84e0c6d..e09b859b698120 100644
--- a/llvm/test/Transforms/Inline/prof-update-sample.ll
+++ b/llvm/test/Transforms/Inline/prof-update-sample.ll
@@ -52,7 +52,6 @@ define void @caller() {
!17 = !{!"branch_weights", i32 400}
!18 = !{!"VP", i32 0, i64 140, i64 111, i64 80, i64 222, i64 40, i64 333, i64 20}
; CHECK: ![[ENTRY_COUNT]] = !{!"function_entry_count", i64 600}
-; CHECK: ![[COUNT_CALLEE1]] = !{!"branch_weights", i32 2000}
; CHECK: ![[COUNT_CALLEE]] = !{!"branch_weights", i32 1200}
; CHECK: ![[COUNT_IND_CALLEE]] = !{!"VP", i32 0, i64 84, i64 111, i64 48, i64 222, i64 24, i64 333, i64 12}
; CHECK: ![[COUNT_CALLER]] = !{!"branch_weights", i32 800}
|
antoniofrighetto
force-pushed
the
feature/cloning-postpone-instsimplify
branch
from
fc09adb
to
7a5218d
Compare
| return const_cast<Value *>(OrigI); | ||
| } | ||
| return nullptr; | ||
| }; |
There was a problem hiding this comment.
This looks very inefficient if many values are cloned and simplified.
There was a problem hiding this comment.
Had the same concern, though in practice I would expect it not to happen that often as most of the times the instruction would be trivially dead? Alternatively, we could iterate over all the instructions of the old function, looking up VMap each time to get the new instruction and see if that can be simplified? Not sure if it’d be worth it though. Compile time does not look excessively bad: http://llvm-compile-time-tracker.com/compare.php?from=d34a2c2adb2a4f1dc262c5756d3725caa4ea2571&to=79475c2bf9493e51dcdf46e49b8bd15da326e912&stat=instructions:u. Also, IIRC RPOT was outperforming classic iteration in a few settings (stage2-O3).
There was a problem hiding this comment.
@nikic, rewrote it as I was not too much happy with iterating over VMap either. It looks overall slightly better now, although it seems this was not the actual issue as the regression on sqlite3 is still there: http://llvm-compile-time-tracker.com/compare.php?from=17b86d5978af8d171fa28763a9e5eba3ce93713a&to=6870569453fb583c81cb1a8ac7ef321dd7d2aaa7&stat=instructions:u. What do you think?
There was a problem hiding this comment.
FWIW avoiding instruction simplification outright leads to overall worse regressions despite some little amelioration here and there: http://llvm-compile-time-tracker.com/compare.php?from=d34a2c2adb2a4f1dc262c5756d3725caa4ea2571&to=f9307dc124eba7bb019310c9de043166e494f7a3&stat=instructions:u.
There was a problem hiding this comment.
It would be good to understand what causes the sqlite3 regression. My theory is that this is because the block cloning is lazy: If we see that the branch condition has folded to a constant, we may only clone one successor.
I think that after this change that logic is essentially dead and we'll always clone the whole function. The whole function might be much larger than just the part reachable via folded conditions. Especially if you take into account that InlineCost does the same thing, so inlining a very large function may be considered cheap if most of it is dead based on caller information.
If this theory is true, then what we might want to do is to still eagerly perform constant folding, and then do the InstSimplify pass afterwards. (We could also test doing only constant folding...)
| auto *OriginalI = GetOriginalValueInVMap(&NewI); | ||
| assert(OriginalI != nullptr && | ||
| "Previously remapped value was not found?"); | ||
| VMap[OriginalI] = &NewI; |
There was a problem hiding this comment.
TBH I don't understand what this code is doing. It seems like a no-op? It finds the VMap entry OriginalI -> &NewI and then replaces it with ... &NewI?
Was this supposed to replace with V? Also why don't we need to do that if we erase the instruction? Won't we leave behind dangling pointers in VMap with the current implementation?
There was a problem hiding this comment.
replaceAllUsesWith will drop the old ValueHandleBase in VMap, and update it with the newly-simplified value via ValueHandleBase::ValueIsRAUWd. If we do not erase the instruction, we have to manually restore NewI. I must have missed something while refactoring the code, since clearly we have to retrieve OriginalI through the just-replaced value V, not via NewI, as VMap state has changed. No need to do anything when the instruction gets erased. Fixed it now, thanks.
antoniofrighetto
force-pushed
the
feature/cloning-postpone-instsimplify
branch
3 times, most recently
from
8d960d1
to
6870569
Compare
antoniofrighetto
force-pushed
the
feature/cloning-postpone-instsimplify
branch
from
6870569
to
8944a84
Compare
|
@nikic, I confirm performing basic constant folding on average decreases the number of blocks being cloned; I think we should be more on track now (http://llvm-compile-time-tracker.com/compare.php?from=eec268e41000faad50b7bf977c9f1161e4006bab&to=65847e8c168123ad328683f96b1b37cd269ea85c&stat=instructions:u). I think we still want to do InstSimplify after cloning, since not doing it leads to regressions in stage2 scenario (http://llvm-compile-time-tracker.com/compare.php?from=60baaf153d05a7828b304050aba461ebb66232c6&to=78c72fd92fa1b22fd2d113bb4e821b484322aadd&stat=instructions:u), besides the following local failures: |
antoniofrighetto
force-pushed
the
feature/cloning-postpone-instsimplify
branch
from
8944a84
to
f646eaa
Compare
|
• s/isInstTriviallyDead/wouldInstBeTriviallyDead/: http://llvm-compile-time-tracker.com/compare.php?from=2a47ee070145438424b065a35c4a680ea0cb0c1f&to=893dc199c447806aeb29436cc9e4dd3878c91bc4&stat=instructions:u |
| if (!NewInst->mayHaveSideEffects()) { | ||
| // Eagerly constant fold the newly cloned instruction. If successful, add | ||
| // a mapping to the new value. | ||
| if (Value *V = ConstantFoldInstruction( |
There was a problem hiding this comment.
Maybe worth explicitly noting that non-constant operands might not incomplete at this point, so we don't want to try to do simplifications based on them.
| // a mapping to the new value. | ||
| if (Value *V = ConstantFoldInstruction( | ||
| NewInst, BB->getModule()->getDataLayout())) { | ||
| if (wouldInstructionBeTriviallyDead(NewInst)) { |
There was a problem hiding this comment.
I'd personally use isInstructionTriviallyDead here just to be more obvious (even though we know that there are no uses at this point.)
antoniofrighetto
force-pushed
the
feature/cloning-postpone-instsimplify
branch
from
f646eaa
to
c455aaa
Compare
A logic issue arose when inlining via `CloneAndPruneFunctionInto`, which, besides cloning, performs instruction simplification 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 variants could lead to unsound optimizations. This issue has been addressed by performing basic constant folding while cloning, and postponing instruction simplification once phi-nodes are revisited. Fixes: llvm#87534.
antoniofrighetto
force-pushed
the
feature/cloning-postpone-instsimplify
branch
from
c455aaa
to
a61f9fe
Compare
|
This patch breaks our downstream CI: dtcxzyw/llvm-opt-benchmark#550 Reduced testcase: opt crashes when processing llvm-project/llvm/lib/Analysis/ValueTracking.cpp Lines 7572 to 7575 in d609029 I don't know why the ret attribute |
|
Multiple bots are broken, maybe revert this patch in the meanwhile? https://lab.llvm.org/buildbot/#/builders/121/builds/41052/steps/10/logs/stdio |
|
The issue looks orthogonal and incidentally happens to be triggered by the patch, I'll take a look soon. |
|
@antoniofrighetto the first rule of buildbots is that they stay green. No matter what the root cause is, if you break the bots, you revert first, and sort it out later. |
…tion" #87963 Reopens #87534. Breaks multiple bots: https://lab.llvm.org/buildbot/#/builders/168/builds/20028 https://lab.llvm.org/buildbot/#/builders/74/builds/27773 And reproducer in a61f9fe. This reverts commit a61f9fe.
|
See #87482 for discussion about this issue. Possibly this change also provide a way to fix that one properly, by delaying simplification until after the returns have been rewritten... |
|
I've noticed that before this PR, certain calls were simplified-away and now they are not because of // Skip over non-intrinsic callsites, we don't want to remove any nodes
// from the CGSCC.
CallBase *CB = dyn_cast<CallBase>(NewI);
if (CB && CB->getCalledFunction() &&
!CB->getCalledFunction()->isIntrinsic())
continue;I'm not clear if this check is still required. And if it is, was the fact that it was missing in CloneBLock before a bug? Shouldn't it still be added there, since ConstantFold can still eliminate some non-intrinsic calls (i.e. some math functions)? It was added by @majnemer in 5554eda |
We do not need to concern ourselves with CGSCC since all remaining CG related updates went away in fa6ea7a as pointed out by @nikic in #87963 (comment).
A logic issue arose when inlining via
CloneAndPruneFunctionInto, which, besides cloning, performs instruction simplification 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. AsVMapstate at this stage is incomplete,threadCmpOverPHIand variants could lead to unsound optimizations. This issue has been addressed by performing basic constant folding while cloning, and postponing instruction simplification once phi-nodes are revisited.Fixes: #87534.