[PATCH] D85837: [GlobalOpt] Fix incorrect Modified status

[Florian Hahn via Phabricator via llvm-commits]

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

LGTM thanks.



================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2027
+    if (GS.Ordering == AtomicOrdering::NotAtomic) {
       GV->setConstant(true);
+      Changed = true;
----------------
dstenb wrote:
> fhahn wrote:
> > 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?
> 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.
thanks for checking. IMO it would be good to add an assertion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85837