[GlobalOpt] Fix another incorrect Modified status

[dstenb]

Summary

When removing a non-constant store to a global in
CleanupPointerRootUsers(), the GlobalOpt pass could incorrectly return
false.

This was caught using the check introduced by D80916.

[efriedma]

llvm/test/Transforms/GlobalOpt/dead-store-status.ll

11

This test passes on master, I think?

[dstenb]

llvm/test/Transforms/GlobalOpt/dead-store-status.ll

11

Yes. The return status check is hidden under EXPENSIVE_CHECKS. With that enabled you get a failed assertion on this test case on master:

Script:
--
: 'RUN: at line 1';   /home/edasten/upstream/monorepo/build-expensive/bin/opt < /home/edasten/upstream/monorepo/llvm/test/Transforms/GlobalOpt/dead-store-status.ll -globalopt -S | /home/edasten/upstream/monorepo/build-expensive/bin/FileCheck /home/edasten/upstream/monorepo/llvm/test/Transforms/GlobalOpt/dead-store-status.ll
--
Exit Code: 2

Command Output (stderr):
--
opt: /home/edasten/upstream/monorepo/llvm/lib/IR/LegacyPassManager.cpp:1706: bool {anonymous}::MPPassManager::runOnModule(llvm::Module&): Assertion `(LocalChanged || (RefHash == StructuralHash(M))) && "Pass modifies its input and doesn't report it."' failed.

Should I perhaps add a comment about that?

[efriedma]

LGTM

llvm/test/Transforms/GlobalOpt/dead-store-status.ll

11

Probably worth adding a short comment noting that this is depending on the EXPENSIVE_CEHCKS check.

Alternatively, have you considered sticking the check under a flag that the user could enable on non-EXPENSIVE_CHECKS builds? We do this in other contexts (e.g. MachineVerifier).

[dstenb]

llvm/test/Transforms/GlobalOpt/dead-store-status.ll

11

Sorry, I have myself not been involved with the introduction of this check; I just encountered this issue and some others when using the check with our downstream target. Any thoughts on the above, @serge-sans-paille?

[dstenb]

I'll land this with a comment added to the test case in the meanwhile.

llvm/lib/Transforms/IPO/GlobalOpt.cpp

if (IsSafeComputationToRemove(Dead[i].first, GetTLI)) {
           break;
         Instruction *J = dyn_cast<Instruction>(I->getOperand(0));
         if (!J)
           break;
         I->eraseFromParent();
         I = J;
       } while (true);
       I->eraseFromParent();
+      Changed = true;
     }
   }
 
   return Changed;
 }
 
 /// We just marked GV constant.  Loop over all users of the global, cleaning up
 /// the obvious ones.  This is largely just a quick scan over the use list to

llvm/test/Transforms/GlobalOpt/dead-store-status.ll

+; RUN: opt < %s -globalopt -S | FileCheck %s
+
+; When removing the store to @global in @foo, the pass would incorrectly return
+; false. This was caught by the pass return status check that is hidden under
+; EXPENSIVE_CHECKS.
+
+; CHECK: @global = internal unnamed_addr global i16* null, align 1
+
+; CHECK-LABEL: @foo
+; CHECK-NEXT: entry:
+; CHECK-NEXT: ret i16 undef
+
+@global = internal unnamed_addr global i16* null, align 1
+
+; Function Attrs: nofree noinline norecurse nounwind writeonly
+define i16 @foo(i16 %c) local_unnamed_addr #0 {
+entry:
+  %local1.addr = alloca i16, align 1
+  store i16* %local1.addr, i16** @global, align 1
+  ret i16 undef
+}
+
+; Function Attrs: noinline nounwind writeonly
+define i16 @bar() local_unnamed_addr #1 {
+entry:
+  %local2 = alloca [1 x i16], align 1
+  %0 = bitcast [1 x i16]* %local2 to i8*
+  call void @llvm.lifetime.start.p0i8(i64 1, i8* nonnull %0)
+  %arraydecay = getelementptr inbounds [1 x i16], [1 x i16]* %local2, i16 0, i16 0
+  store i16* %arraydecay, i16** @global, align 1
+  call void @llvm.lifetime.end.p0i8(i64 1, i8* nonnull %0)
+  ret i16 undef
+}
+
+; Function Attrs: argmemonly nounwind willreturn
+declare void @llvm.lifetime.start.p0i8(i64 immarg, i8* nocapture) #2
+
+; Function Attrs: argmemonly nounwind willreturn
+declare void @llvm.lifetime.end.p0i8(i64 immarg, i8* nocapture) #2
+
+attributes #0 = { nofree noinline norecurse nounwind writeonly }
+attributes #1 = { noinline nounwind writeonly }
+attributes #2 = { argmemonly nounwind willreturn }