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

[David Stenberg via Phabricator via llvm-commits]

dstenb added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:2027
+    if (GS.Ordering == AtomicOrdering::NotAtomic) {
       GV->setConstant(true);
+      Changed = true;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85837