[GlobalOpt] Handle operators separately when removing GV users #84694

gandhi56 · 2024-03-10T21:21:51Z

Refactor globalopt by eliminating redundant code. Fix #64680.

llvmbot · 2024-03-10T21:22:22Z

@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-llvm-transforms

Author: Anshil Gandhi (gandhi56)

Changes

When CleanupPointerRootUsers is called on GV,
the assumption is that there is no load from GV.
Erase all stores to GV in this case.

Fixes #64680.

Full diff: https://github.com/llvm/llvm-project/pull/84694.diff

3 Files Affected:

(modified) llvm/lib/Transforms/IPO/GlobalOpt.cpp (+9-1)
(added) llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-stores.ll (+62)
(modified) llvm/test/Transforms/GlobalOpt/dead-store-status.ll (-2)

diff --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index da714c9a75701b..2b3154eeb6100f 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -204,8 +204,9 @@ CleanupPointerRootUsers(GlobalVariable *GV,
   // If Dead[n].first is the only use of a malloc result, we can delete its
   // chain of computation and the store to the global in Dead[n].second.
   SmallVector<std::pair<Instruction *, Instruction *>, 32> Dead;
-
+  SmallVector<StoreInst *> StoresToGV;
   SmallVector<User *> Worklist(GV->users());
+
   // Constants can't be pointers to dynamically allocated memory.
   while (!Worklist.empty()) {
     User *U = Worklist.pop_back_val();
@@ -217,6 +218,8 @@ CleanupPointerRootUsers(GlobalVariable *GV,
       } else if (Instruction *I = dyn_cast<Instruction>(V)) {
         if (I->hasOneUse())
           Dead.push_back(std::make_pair(I, SI));
+        else
+          StoresToGV.push_back(SI);
       }
     } else if (MemSetInst *MSI = dyn_cast<MemSetInst>(U)) {
       if (isa<Constant>(MSI->getValue())) {
@@ -241,6 +244,11 @@ CleanupPointerRootUsers(GlobalVariable *GV,
     }
   }
 
+  // We assert here that GV is never load from so
+  // we can safely remove all stores to GV.
+  for (StoreInst *SI : StoresToGV)
+    SI->eraseFromParent();
+
   for (int i = 0, e = Dead.size(); i != e; ++i) {
     if (IsSafeComputationToRemove(Dead[i].first, GetTLI)) {
       Dead[i].second->eraseFromParent();
diff --git a/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-stores.ll b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-stores.ll
new file mode 100644
index 00000000000000..980fb6f5dc8bfe
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-stores.ll
@@ -0,0 +1,62 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
+; RUN: opt -passes=globalopt < %s -S | FileCheck %s
+
+@a = internal unnamed_addr global i32 0, align 4
+@b = internal unnamed_addr global [3 x ptr] zeroinitializer, align 16
+
+define i32 @main() local_unnamed_addr {
+; CHECK-LABEL: define i32 @main() local_unnamed_addr {
+; CHECK-NEXT:  entry:
+; CHECK-NEXT:    [[E:%.*]] = alloca i32, align 4
+; CHECK-NEXT:    [[DOTPR:%.*]] = load i32, ptr @a, align 4
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i32 [[DOTPR]], 3
+; CHECK-NEXT:    br i1 [[CMP1]], label [[FOR_BODY:%.*]], label [[FOR_END:%.*]]
+; CHECK:       for.body:
+; CHECK-NEXT:    store i32 8, ptr [[E]], align 4
+; CHECK-NEXT:    call void @bar20_()
+; CHECK-NEXT:    [[TMP0:%.*]] = load i32, ptr [[E]], align 4
+; CHECK-NEXT:    [[TOBOOL_NOT:%.*]] = icmp eq i32 [[TMP0]], 0
+; CHECK-NEXT:    br i1 [[TOBOOL_NOT]], label [[IF_THEN:%.*]], label [[IF_END:%.*]]
+; CHECK:       if.then:
+; CHECK-NEXT:    call void @foo()
+; CHECK-NEXT:    br label [[IF_END]]
+; CHECK:       if.end:
+; CHECK-NEXT:    [[TMP1:%.*]] = load i32, ptr @a, align 4
+; CHECK-NEXT:    [[INC:%.*]] = add nsw i32 [[TMP1]], 1
+; CHECK-NEXT:    store i32 [[INC]], ptr @a, align 4
+; CHECK-NEXT:    [[CMP:%.*]] = icmp slt i32 [[TMP1]], 2
+; CHECK-NEXT:    br i1 [[CMP]], label [[FOR_BODY]], label [[FOR_END]]
+; CHECK:       for.end:
+; CHECK-NEXT:    ret i32 0
+;
+entry:
+  %e = alloca i32, align 4
+  %.pr = load i32, ptr @a, align 4
+  %cmp1 = icmp slt i32 %.pr, 3
+  br i1 %cmp1, label %for.body, label %for.end
+
+for.body:                                         ; preds = %entry, %if.end
+  store i32 8, ptr %e, align 4
+  call void @bar20_()
+  %0 = load i32, ptr %e, align 4
+  %tobool.not = icmp eq i32 %0, 0
+  br i1 %tobool.not, label %if.then, label %if.end
+
+if.then:                                          ; preds = %for.body
+  call void @foo()
+  br label %if.end
+
+if.end:                                           ; preds = %if.then, %for.body
+  store ptr %e, ptr getelementptr inbounds ([3 x ptr], ptr @b, i64 0, i64 2), align 16
+  %1 = load i32, ptr @a, align 4
+  %inc = add nsw i32 %1, 1
+  store i32 %inc, ptr @a, align 4
+  %cmp = icmp slt i32 %1, 2
+  br i1 %cmp, label %for.body, label %for.end
+
+for.end:                                          ; preds = %if.end, %entry
+  ret i32 0
+}
+
+declare void @bar20_() local_unnamed_addr
+declare void @foo() local_unnamed_addr
diff --git a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
index 9a8fbb8d65f0e0..597c08929af902 100644
--- a/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
+++ b/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
@@ -4,8 +4,6 @@
 ; false. This was caught by the pass return status check that is hidden under
 ; EXPENSIVE_CHECKS.
 
-; CHECK: @global = internal unnamed_addr global ptr null, align 1
-
 ; CHECK-LABEL: @foo
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT: ret i16 undef

gandhi56 · 2024-03-12T06:29:19Z

I see a flaw in my solution, I will update this PR with a more effective solution. Stay tuned!

gandhi56 · 2024-03-13T19:02:08Z

Internal CI passed.

gandhi56 · 2024-03-15T07:11:13Z

ping

gandhi56 · 2024-03-21T08:01:13Z

@nikic @fhahn ping

gandhi56 · 2024-04-01T06:21:52Z

Ping

PR Link: llvm/llvm-project#84694

llvm/lib/Transforms/IPO/GlobalOpt.cpp

fhahn

Please clarify the commit title and description to provide more details on the change. Refactor CleanupPointerRootUsers makes it sound like there's no change in functionality, but IIUC this adds new functionality.

llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-loads.ll

gandhi56 · 2024-10-29T12:54:45Z

Internal CI passed

gandhi56 · 2024-11-06T05:36:37Z

@efriedma-quic Is anyone familiar with how to run valgrind tests? Or any other comments on this PR?

efriedma-quic

I think the testcase is just running a program that stores the result of a malloc to an internal global variable under valgrind. I don't have any specific examples handy; maybe someone else does.

efriedma-quic · 2024-11-06T21:48:09Z

llvm/lib/Transforms/IPO/GlobalOpt.cpp

+        continue;
+      if (isa<Instruction, Operator>(Src)) {
+        EraseFromParent(MTI);
+        if (isSafeComputationToRemove(Src, GetTLI))


I was trying to say that you can just call RecursivelyDeleteTriviallyDeadInstructions without checking if the instruction is dead; it internally checks if the instruction is dead.

But more importantly, it means isSafeComputationToRemove isn't actually doing anything useful, so the end result is that this behaves identically to D69428.

gandhi56 · 2024-11-07T10:12:22Z

@efriedma-quic I took a second look through all the cleanup routines. isLeakCheckerRoot(GV) seems necessary to me even though it's rarely applicable. GV may contain pointers which should be handled more carefully than simply calling RecursivelyDeleteTriviallyDeadInstructions(..). So I am thinking to keep cleanupPointerRootUsers and improve it to cover cases as such as in the issue #64680. This will also prevent any potential failures in valgrind tests.

efriedma-quic · 2024-11-07T20:13:51Z

Deleting stores where we can prove the stored value isn't a pointer to an allocation should be fine; it doesn't impact the controversial cases. (But the current version of the patch is more aggressive, maybe by accident.)

gandhi56 · 2024-11-09T06:25:01Z

Deleting stores where we can prove the stored value isn't a pointer to an allocation should be fine; it doesn't impact the controversial cases. (But the current version of the patch is more aggressive, maybe by accident.)

Assuming GV is a pointer root which is never read from, the store instruction can be removed even if it's value operand is a pointer.

I think the logic implemented to decide whether or not source value operands should be removed was a little messy. Hence the test from #64680 was failing.

gandhi56 · 2024-11-13T13:48:35Z

ping

vitalybuka · 2024-11-14T20:22:24Z

ping

Assuming you are expecting @efriedma-quic reply, don't forget "note" from https://llvm.org/docs/CodeReview.html#acknowledge-all-reviewer-feedback

gandhi56 · 2024-12-13T17:32:38Z

ping

Remove CleanupPointerRootUsers and isLeakCheckerRoot. Replace calls to CleanupPointerRootUsers with an extended version of CleanupConstantGlobalUsers. Fixes llvm#64680. Change-Id: I4c3fc52890eec1a3706cba9ecb916fd344672756

gandhi56 · 2025-03-22T05:13:09Z

ping

#84694)" This reverts commit 51dad71.

…s" (#132971) Reverts #84694 . Review was incomplete.

efriedma-quic · 2025-03-25T18:31:47Z

If you patch isn't getting appropriate review, merging it is not an appropriate way to escalate. Appropriate ways include privately emailing reviewers, or starting a thread on Discourse.

I apologize for not continuing to review this in a timely fashion. I should not have ignored the patch like this, after I started reviewing.

There are basically two ways forward for this patch: one, you preserve the "leak root" check so that allocation checking tools don't break. Two, we explicitly decide to break any tools that depend on "leak root" checking (probably needs an RFC). Your patch is currently doing neither one of those; it's still trying to straddle the middle in a way I can't understand.

gandhi56 · 2025-03-25T18:37:08Z

I am sorry about that, I thought I saw an approval. I really appreciate your comments, I will work on a new PR to resolve this issue.

…ing GV users" (#132971) Reverts llvm/llvm-project#84694 . Review was incomplete.

nikic · 2025-04-02T10:26:56Z

FWIW, my opinion on what to do here is #109718 (comment).

gandhi56 requested review from nikic and fhahn March 10, 2024 21:21

llvmbot added the llvm:transforms label Mar 10, 2024

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch from eaa2602 to 3b9a9d6 Compare March 11, 2024 01:33

gandhi56 requested a review from Sterling-Augustine March 11, 2024 15:35

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch from 3b9a9d6 to b71057a Compare March 13, 2024 05:59

gandhi56 changed the title ~~[GlobalOpt] Remove all stores to GV~~ [GlobalOpt] Refactor CleanupPointerRootUsers Mar 13, 2024

gandhi56 self-assigned this Mar 13, 2024

gandhi56 requested review from TNorthover and echristo March 17, 2024 06:41

arsenm added the ipo Interprocedural optimizations label Mar 19, 2024

gandhi56 requested review from jmolloy, hfinkel and bwendling March 26, 2024 16:25

dtcxzyw added a commit to dtcxzyw/llvm-opt-benchmark that referenced this pull request Apr 1, 2024

pre-commit: test PR84694

19e6968

PR Link: llvm/llvm-project#84694

dtcxzyw mentioned this pull request Apr 1, 2024

pre-commit: test PR84694 dtcxzyw/llvm-opt-benchmark#466

Closed

dtcxzyw requested changes Apr 1, 2024

View reviewed changes

llvm/lib/Transforms/IPO/GlobalOpt.cpp Show resolved Hide resolved

fhahn reviewed Apr 2, 2024

View reviewed changes

llvm/test/Transforms/GlobalOpt/cleanup-pointer-root-loads.ll Outdated Show resolved Hide resolved

gandhi56 changed the title ~~[GlobalOpt] Refactor CleanupPointerRootUsers~~ [GlobalOpt] Handle operators separately when removing GV users Apr 2, 2024

gandhi56 mentioned this pull request Apr 3, 2024

[GlobalOpt] Precommit tests for PR84694 #87443

Merged

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch 4 times, most recently from 4b6f1ca to 80ba2b7 Compare April 3, 2024 03:54

efriedma-quic mentioned this pull request Sep 24, 2024

[clang] Missed optimization: dead store to thread-local not optimized out #109718

Open

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch from bd76371 to fe47475 Compare October 24, 2024 21:38

efriedma-quic requested review from MaskRay, vitalybuka and jyknight November 6, 2024 21:39

efriedma-quic reviewed Nov 6, 2024

View reviewed changes

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch 2 times, most recently from b7e7d55 to 82f091c Compare November 8, 2024 14:35

vitalybuka removed their request for review November 14, 2024 20:22

vitalybuka requested a review from efriedma-quic December 29, 2024 07:28

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch from 82f091c to 67a0a21 Compare January 7, 2025 18:41

[GlobalOpt] Eliminate redundant cleanup functions

39a1484

Remove CleanupPointerRootUsers and isLeakCheckerRoot. Replace calls to CleanupPointerRootUsers with an extended version of CleanupConstantGlobalUsers. Fixes llvm#64680. Change-Id: I4c3fc52890eec1a3706cba9ecb916fd344672756

gandhi56 force-pushed the globalopt-cleanup-ptr-root-users-64680 branch from 67a0a21 to 39a1484 Compare January 7, 2025 19:01

gandhi56 merged commit 51dad71 into llvm:main Mar 25, 2025
8 checks passed

efriedma-quic added a commit that referenced this pull request Mar 25, 2025

Revert "[GlobalOpt] Handle operators separately when removing GV users (

244daaf

#84694)" This reverts commit 51dad71.

efriedma-quic mentioned this pull request Mar 25, 2025

Revert "[GlobalOpt] Handle operators separately when removing GV users" #132971

Merged

efriedma-quic added a commit that referenced this pull request Mar 25, 2025

Revert "[GlobalOpt] Handle operators separately when removing GV user…

e16e93a

…s" (#132971) Reverts #84694 . Review was incomplete.

llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this pull request Mar 25, 2025

Automerge: Revert "[GlobalOpt] Handle operators separately when remov…

39bb9b9

…ing GV users" (#132971) Reverts llvm/llvm-project#84694 . Review was incomplete.

[GlobalOpt] Handle operators separately when removing GV users #84694

[GlobalOpt] Handle operators separately when removing GV users #84694

Uh oh!

Conversation

gandhi56 commented Mar 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented Mar 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gandhi56 commented Mar 12, 2024

Uh oh!

gandhi56 commented Mar 13, 2024

Uh oh!

gandhi56 commented Mar 15, 2024

Uh oh!

gandhi56 commented Mar 21, 2024

Uh oh!

gandhi56 commented Apr 1, 2024

Uh oh!

Uh oh!

fhahn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gandhi56 commented Oct 29, 2024

Uh oh!

gandhi56 commented Nov 6, 2024

Uh oh!

efriedma-quic left a comment

Choose a reason for hiding this comment

Uh oh!

efriedma-quic Nov 6, 2024

Choose a reason for hiding this comment

Uh oh!

gandhi56 commented Nov 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efriedma-quic commented Nov 7, 2024

Uh oh!

gandhi56 commented Nov 9, 2024

Uh oh!

gandhi56 commented Nov 13, 2024

Uh oh!

vitalybuka commented Nov 14, 2024

Uh oh!

gandhi56 commented Dec 13, 2024

Uh oh!

gandhi56 commented Mar 22, 2025

Uh oh!

Uh oh!

efriedma-quic commented Mar 25, 2025

Uh oh!

gandhi56 commented Mar 25, 2025

Uh oh!

nikic commented Apr 2, 2025

Uh oh!

Uh oh!

gandhi56 commented Mar 10, 2024 •

edited

Loading

llvmbot commented Mar 10, 2024 •

edited

Loading

gandhi56 commented Nov 7, 2024 •

edited

Loading