[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:1063
+        if (SI->getOperand(0) == V &&
+            SI->getOperand(1)->stripPointerCasts() != GV)
+          return false; // Storing the pointer itself... bad.
----------------
scui wrote:
> efriedma wrote:
> > scui wrote:
> > > efriedma wrote:
> > > > scui wrote:
> > > > > efriedma wrote:
> > > > > > `VUse.getOperandNo() == 0`?
> > > > > Thanks for the suggestion. This is the original code as it is, I can change it as you suggested - but looks to me no difference as SI is available.
> > > > The `SI->getOperand(1)->stripPointerCasts() != GV` seems very suspicious to me; I don't understand why we want to special-case storing the address of a global to itself.
> > > > 
> > > > For the `SI->getOperand(0) == V` part, I guess it doesn't really matter.
> > > I think this is to make sure the malloc site for GV is only for GV.
> > In case it wasn't clear, I meant, "For the SI->getOperand(0) == V part, I guess it doesn't really matter how you write it.  SI->getOperand(0) == V is essentially equivalent to VUse.getOperandNo() == 0."  
> > 
> > I don't follow your explanation for the `SI->getOperand(1)->stripPointerCasts() != GV` part; can you describe it in a bit more detail?
> V is from malloc. For stores, V can be used in the following 3 forms:
> 1. store x, V
> 2. store V, GV
> 3. store V, not-GV
> Case 1 is ok, as it stores through it. Case 2 is the one we care about - store to GV - we are trying to optimize this kind of malloc calls.  This check is for case 3 - the malloc value is also stored to memory other than GV - simply return false, as there is no further code currently to handle this for safe transformation.
Oh... right.  Makes sense.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:735
+        // Ignore stores to the global.
+        if (SI->getPointerOperand() != P)
+          return false;
----------------
Instead of `SI->getPointerOperand() != P`, should be `SI->getValueOperand() == P`?


================
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);
----------------
Is this going to affect other transforms that use the result of analyzeGlobalAux?


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

https://reviews.llvm.org/D106589