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

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86149