[PATCH] D86149: [GlobalOpt] Fix another incorrect Modified status

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/test/Transforms/GlobalOpt/dead-store-status.ll:10
+; CHECK-NEXT: entry:
+; CHECK-NEXT: ret i16 undef
+
----------------
dstenb wrote:
> efriedma wrote:
> > This test passes on master, I think?
> 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?
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).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86149/new/

https://reviews.llvm.org/D86149