[llvm] 6d36b22 - [GlobalOpt] Fix an incorrect Modified status

[David Stenberg via llvm-commits]

Author: David Stenberg
Date: 2020-09-02T15:00:45+02:00
New Revision: 6d36b22b219f663b9b8317147ea8f7a9cb4e18dc

URL: https://github.com/llvm/llvm-project/commit/6d36b22b219f663b9b8317147ea8f7a9cb4e18dc
DIFF: https://github.com/llvm/llvm-project/commit/6d36b22b219f663b9b8317147ea8f7a9cb4e18dc.diff

LOG: [GlobalOpt] Fix an incorrect Modified status

When marking a global variable constant, and simplifying users using
CleanupConstantGlobalUsers(), the pass could incorrectly return false if
there were still some uses left, and no further optimizations was done.

This was caught using the check introduced by D80916.

This fixes PR46749.

Reviewed By: fhahn

Differential Revision: https://reviews.llvm.org/D85837

Added: 
    llvm/test/Transforms/GlobalOpt/const-return-status-atomic.ll
    llvm/test/Transforms/GlobalOpt/const-return-status.ll

Modified: 
    llvm/lib/Transforms/IPO/GlobalOpt.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/GlobalOpt.cpp b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
index 05d1465b3663..f3053398cd5a 100644
--- a/llvm/lib/Transforms/IPO/GlobalOpt.cpp
+++ b/llvm/lib/Transforms/IPO/GlobalOpt.cpp
@@ -1990,12 +1990,13 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
     return true;
   }
 
+  bool Changed = false;
+
   // If the global is never loaded (but may be stored to), it is dead.
   // Delete it now.
   if (!GS.IsLoaded) {
     LLVM_DEBUG(dbgs() << "GLOBAL NEVER LOADED: " << *GV << "\n");
 
-    bool Changed;
     if (isLeakCheckerRoot(GV)) {
       // Delete any constant stores to the global.
       Changed = CleanupPointerRootUsers(GV, GetTLI);
@@ -2021,11 +2022,14 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
     // Don't actually mark a global constant if it's atomic because atomic loads
     // are implemented by a trivial cmpxchg in some edge-cases and that usually
     // requires write access to the variable even if it's not actually changed.
-    if (GS.Ordering == AtomicOrdering::NotAtomic)
+    if (GS.Ordering == AtomicOrdering::NotAtomic) {
+      assert(!GV->isConstant() && "Expected a non-constant global");
       GV->setConstant(true);
+      Changed = true;
+    }
 
     // Clean up any obviously simplifiable users now.
-    CleanupConstantGlobalUsers(GV, GV->getInitializer(), DL, GetTLI);
+    Changed |= CleanupConstantGlobalUsers(GV, GV->getInitializer(), DL, GetTLI);
 
     // If the global is dead now, just nuke it.
     if (GV->use_empty()) {
@@ -2085,7 +2089,7 @@ processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
     }
   }
 
-  return false;
+  return Changed;
 }
 
 /// Analyze the specified global variable and optimize it if possible.  If we

diff  --git a/llvm/test/Transforms/GlobalOpt/const-return-status-atomic.ll b/llvm/test/Transforms/GlobalOpt/const-return-status-atomic.ll
new file mode 100644
index 000000000000..f52ba05e6c19
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/const-return-status-atomic.ll
@@ -0,0 +1,27 @@
+; RUN: opt -globalopt < %s -S -o - | FileCheck %s
+
+; When simplifying users of a global variable, the pass could incorrectly
+; return false if there were still some uses left, and no further optimizations
+; was done. This was caught by the pass return status check that is hidden
+; under EXPENSIVE_CHECKS.
+
+ at GV1 = internal unnamed_addr global i64 1, align 8
+
+; CHECK: @GV1 = internal unnamed_addr global i64 1, align 8
+
+define void @test1() local_unnamed_addr {
+; CHECK-LABEL: @test1
+; CHECK-NEXT: %val = load atomic i8
+; CHECK-NEXT: ret void
+
+  %val = load atomic i8, i8* bitcast (i64* @GV1 to i8*) acquire, align 8
+  ret void
+}
+
+define i64 @test2() local_unnamed_addr {
+; CHECK-LABEL: @test2
+; CHECK-NEXT: ret i64 1
+
+  %val = load atomic i64, i64* @GV1 acquire, align 8
+  ret i64 %val
+}

diff  --git a/llvm/test/Transforms/GlobalOpt/const-return-status.ll b/llvm/test/Transforms/GlobalOpt/const-return-status.ll
new file mode 100644
index 000000000000..32c4eb895dc1
--- /dev/null
+++ b/llvm/test/Transforms/GlobalOpt/const-return-status.ll
@@ -0,0 +1,28 @@
+; RUN: opt -globalopt < %s -S -o - | FileCheck %s
+
+; When simplifying users of a global variable, the pass could incorrectly
+; return false if there were still some uses left, and no further optimizations
+; was done. This was caught by the pass return status check that is hidden
+; under EXPENSIVE_CHECKS.
+
+; CHECK: @src = internal unnamed_addr constant
+
+; CHECK: entry:
+; CHECK-NEXT: %call = call i32 @f(i32 0)
+; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 bitcast (i32* @dst to i8*), i8* align 4 bitcast ([1 x i32]* @src to i8*), i64 1, i1 false)
+; CHECK-NEXT: ret void
+
+ at src = internal unnamed_addr global [1 x i32] zeroinitializer, align 4
+ at dst = external dso_local local_unnamed_addr global i32, align 4
+
+define dso_local void @d() local_unnamed_addr {
+entry:
+  %0 = load i32, i32* getelementptr inbounds ([1 x i32], [1 x i32]* @src, i64 0, i64 0), align 4
+  %call = call i32 @f(i32 %0)
+  call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 4 bitcast (i32* @dst to i8*), i8* align 4 bitcast ([1 x i32]* @src to i8*), i64 1, i1 false)
+  ret void
+}
+
+declare dso_local i32 @f(i32) local_unnamed_addr
+
+declare void @llvm.memcpy.p0i8.p0i8.i64(i8* noalias nocapture writeonly, i8* noalias nocapture readonly, i64, i1 immarg)