[GlobalOpt] Fix incorrect Modified status

[dstenb]

Summary

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.

[dstenb]

Ping.

[fhahn]

llvm/lib/Transforms/IPO/GlobalOpt.cpp

2027–2029

Can GV here be already a constant? If so, we should only set Changed if !GV->isConstant(). Or if it is always non-constant to start with, perhaps add an assert to make sure?

[dstenb]

llvm/lib/Transforms/IPO/GlobalOpt.cpp

2027–2029

This can currently not occur, since processGlobal() bails out before calling processInternalGlobal in that case:

if (GVar->isConstant() || !GVar->hasInitializer())
  return Changed;

return processInternalGlobal(GVar, GS, GetTLI, LookupDomTree) || Changed;

I can add an assert to ensure that that contract is upheld between two functions.

[fhahn]

LGTM thanks.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

2027–2029

thanks for checking. IMO it would be good to add an assertion.

[dstenb]

Thanks for the review!

I landed this with an assert:

+    if (GS.Ordering == AtomicOrdering::NotAtomic) {
+      assert(!GV->isConstant() && "Expected a non-constant global");
       GV->setConstant(true);
+      Changed = true;
+    }

llvm/lib/Transforms/IPO/GlobalOpt.cpp

if (!GS.HasMultipleAccessingFunctions &&
     makeAllConstantUsesInstructions(GV);
 
     GV->replaceAllUsesWith(Alloca);
     GV->eraseFromParent();
     ++NumLocalized;
     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);
     } else {
       // Delete any stores we can find to the global.  We may not be able to
       // make it completely dead though.
       Changed =
           CleanupConstantGlobalUsers(GV, GV->getInitializer(), DL, GetTLI);
processInternalGlobal(GlobalVariable *GV, const GlobalStatus &GS,
 
   }
   if (GS.StoredType <= GlobalStatus::InitializerStored) {
     LLVM_DEBUG(dbgs() << "MARKING CONSTANT: " << *GV << "\n");
 
     // 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()) {
       LLVM_DEBUG(dbgs() << "   *** Marking constant allowed us to simplify "
                         << "all users and delete global!\n");
       GV->eraseFromParent();
       ++NumDeleted;
       return true;
if (Constant *SOVConstant = dyn_cast<Constant>(GS.StoredOnceValue)) {
         if (TryToShrinkGlobalToBoolean(GV, SOVConstant)) {
           ++NumShrunkToBool;
           return true;
         }
       }
     }
   }
 
-  return false;
+  return Changed;
 }
 
 /// Analyze the specified global variable and optimize it if possible.  If we
 /// make a change, return true.
 static bool
 processGlobal(GlobalValue &GV,
               function_ref<TargetLibraryInfo &(Function &)> GetTLI,
               function_ref<DominatorTree &(Function &)> LookupDomTree) {

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

+; 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.
+
+@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
+}

llvm/test/Transforms/GlobalOpt/const-return-status.ll

+; 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
+
+@src = internal unnamed_addr global [1 x i32] zeroinitializer, align 4
+@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)