[PATCH] D106589: [GlobalOpt] support ConstantExpr use of global address for OptimizeGlobalAddressOfMalloc

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:735
+        // Ignore stores to the global.
+        if (SI->getPointerOperand() != P)
+          return false;
----------------
scui wrote:
> efriedma wrote:
> > Instead of `SI->getPointerOperand() != P`, should be `SI->getValueOperand() == P`?
> SI->getPointerOperand() != P is alright. It's ok to store into global (actually the StoredOnce).
My concern is that with opaque pointers, it will become possible to write something like `store ptr @g, ptr @g`, at which point the difference between `SI->getPointerOperand() != P` and `SI->getValueOperand() == P` will start to matter.


================
Comment at: llvm/lib/Transforms/Utils/GlobalStatus.cpp:111
+            Ptr = Ptr->stripPointerCasts();
+          if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Ptr)) {
             Value *StoredVal = SI->getOperand(0);
----------------
scui wrote:
> efriedma wrote:
> > Is this going to affect other transforms that use the result of analyzeGlobalAux?
> I only see one user of GlobalStatus - that is processGlobal in GlobalOpt.cpp. I checked all the transformations under this function. Other than tryToOptimizeStoreOfMallocToGlobal,  this added code should be ok for all other transformations, either ConstantExpr is handled already or bailout early if there is any use of ConstantExpr. There is only one to bailout early - TryToShrinkGlobalToBoolean -  I'll leave this as it is for now as I don't see any real case for this with ConstantExpr. 
> 
Okay, thanks for checking.


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

https://reviews.llvm.org/D106589