-
Notifications
You must be signed in to change notification settings - Fork 14.9k
[SCCP][FuncSpec] Poison unreachable constant global variable user #155753
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
Conversation
@llvm/pr-subscribers-function-specialization @llvm/pr-subscribers-llvm-transforms Author: XChy (XChy) ChangesThe original assumption "we already replaced its users with a constant" for the global variable becomes incorrect after #154668. The users in the dead function are not simplified, in fact. Full diff: https://github.com/llvm/llvm-project/pull/155753.diff 2 Files Affected:
diff --git a/llvm/lib/Transforms/IPO/SCCP.cpp b/llvm/lib/Transforms/IPO/SCCP.cpp
index e98a70f228ada..f4961faf7ad18 100644
--- a/llvm/lib/Transforms/IPO/SCCP.cpp
+++ b/llvm/lib/Transforms/IPO/SCCP.cpp
@@ -330,12 +330,15 @@ static bool runIPSCCP(
LLVM_DEBUG(dbgs() << "Found that GV '" << GV->getName()
<< "' is constant!\n");
for (User *U : make_early_inc_range(GV->users())) {
- // We can remove LoadInst here, because we already replaced its users
- // with a constant.
+ // We can remove LoadInst here. The LoadInsts in dead functions marked by
+ // FuncSpec are not simplified to constants, thus poison them.
assert((isa<StoreInst>(U) || isa<LoadInst>(U)) &&
"Only Store|Load Instruction can be user of GlobalVariable at "
"reaching here.");
- cast<Instruction>(U)->eraseFromParent();
+ Instruction *I = cast<Instruction>(U);
+ if (isa<LoadInst>(I))
+ I->replaceAllUsesWith(PoisonValue::get(I->getType()));
+ I->eraseFromParent();
}
// Try to create a debug constant expression for the global variable
diff --git a/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll b/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll
new file mode 100644
index 0000000000000..29c052a5a9bac
--- /dev/null
+++ b/llvm/test/Transforms/FunctionSpecialization/dead-gv-load.ll
@@ -0,0 +1,31 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -passes=ipsccp --funcspec-min-function-size=1 -S < %s | FileCheck %s
+
+@gv = internal global ptr null
+
+define i8 @caller() {
+; CHECK-LABEL: define i8 @caller() {
+; CHECK-NEXT: [[ENTRY:.*:]]
+; CHECK-NEXT: [[CALL1:%.*]] = call i8 @callee.specialized.1(i32 1)
+; CHECK-NEXT: [[CALL2:%.*]] = call i8 @callee.specialized.2(i32 0)
+; CHECK-NEXT: ret i8 undef
+;
+entry:
+ %call1 = call i8 @callee(i32 1)
+ %call2 = call i8 @callee(i32 0)
+ ret i8 %call2
+}
+
+define internal i8 @callee(i32 %arg) {
+entry:
+ %useless = icmp ne i32 %arg, 0
+ br label %loop
+
+loop: ; preds = %cleanup424, %entry
+ br label %loop
+
+dead_bb: ; No predecessors!
+ %l1 = load ptr, ptr @gv, align 8
+ %l2 = load ptr, ptr %l1, align 8
+ ret i8 0
+}
|
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.
LGTM, thanks!
…oject into bugprone-method-hiding * 'bugprone-method-hiding' of github.com:t-a-james/llvm-project: (230 commits) [SimplifyCFG] Move token type check into canReplaceOperandWithVariable() [ADT] Fix signed integer overflow (llvm#155826) [Offload] Update LIBOMPTARGET_INFO text for `attach` map-type. (llvm#155509) [CMake][AIX] Enable CMP0182: Create shared library archives by default (llvm#155686) AMDGPU: Add tests for atomics with AGPR operands (llvm#155820) [AArch64] Split zero cycle zeoring per register class (llvm#154561) [gn build] Port fa883e1 [mlir][tosa] Allow shift operand of tosa::MulOp as non-constant (llvm#155197) [AArch64][NFC] Add MCInstrAnalysis unittests (llvm#155609) [Offload][OpenMP] Tests require libc on GPU for printf (llvm#155785) AMDGPU: Add missing verifier tests for load/store AGPR case (llvm#155815) [lldb-mcp] Fix building for Windows Revert "[lldb] Correct a usage after a rename was merged. (llvm#155720)" Revert "[lldb] NFC Moving mcp::Transport into its own file. (llvm#155711)" [lldb][test] Run ranges::ref_vew test only for libc++ (llvm#155813) [SCCP][FuncSpec] Poison unreachable constant global variable user (llvm#155753) [LoongArch] Lowering v32i8 vector mask generation to `VMSKLTZ` (llvm#149953) [flang][docs][NFC] Remove stray backtick (llvm#154974) [MLIR] Apply clang-tidy fixes for misc-use-internal-linkage in LinalgOps.cpp (NFC) [MLIR] Apply clang-tidy fixes for performance-move-const-arg in VariantValue.cpp (NFC) ...
Fixes #155738.
The original assumption "we already replaced its users with a constant" for the global variable becomes incorrect after #154668. The users in the dead function are not simplified, in fact.
This patch poisons all the unsimplified constant global variable users.