[PATCH] D101245: [GlobalOpt] Disable heap SROA when GEP of the only storer is used

[Arthur Eubanks via Phabricator via llvm-commits]

aeubanks added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/GlobalOpt.cpp:1016
-    // Must index into the array and into the struct.
-    if (isa<GetElementPtrInst>(Inst) && Inst->getNumOperands() >= 3) {
-      if (!ValueIsOnlyUsedLocallyOrStoredToOneGlobal(Inst, GV, PHIs))
----------------
MaskRay wrote:
> aeubanks wrote:
> > MaskRay wrote:
> > > aeubanks wrote:
> > > > seems like this doesn't take into account when the global variable is an array.
> > > > normally, if we just have a normal struct, the GEP's first operand is the GV, second operand is something like 0, and the third operand indexes into the struct itself, which is fine for SROA. having fewer than 3 operands can directly reference the struct
> > > > with an array, we need to account for an extra index to index into the array
> > > > 
> > > Do you have specific suggestion to the diff? :)
> > > 
> > > I am thinking the value of array heap SROA is low but I don't want to just remove the implementation, so drop GEP - see the tests like heap-sra-phi.ll, heap SROA still works if the function allocates and forgets the array.
> > maybe check the GV type to see if it's an array, and in that case use 4 instead of 3?
> > supporting GEPs seems natural, I'm not sure why we wouldn't want to, seems like code would probably use GEPs into global values to do things
> For `heap-sra-negative.ll`, GV is `@X = internal global %struct.foo* null` (decayed array). `Inst->getNumOperands() >= 4` cannot catch the case.
> 
> This function is only used to check `CI` - the only store instruction. Load instructions are not subject to the constraint, so dropping the support here will not punish the load site GEP.
`Inst->getNumOperands() >= 4` does catch the case, heap SROA doesn't trigger in your test case when we change 3 to 4.
I'm pretty sure "// Must index into the array and into the struct." is what I explained in the first comment of this thread.

> This function is only used to check CI - the only store instruction.
`CI` is the malloc call, not the store (unless I'm misunderstanding your comment).

> Load instructions are not subject to the constraint, so dropping the support here will not punish the load site GEP.
I don't understand. With this patch, if there's a GEP of the malloc anywhere, even if it's used for a load, it gives up.

But it does look like you're right and this is probably never actually hit in practice. Running your example through -O2, the store gets split by instcombine and the global isn't considered "once stored" when GlobalOpt runs. I bootstrapped LLVM with a crash in `PerformHeapAllocSRoA` and it was never hit. We should either go with the 3 -> 4, or completely remove heap SROA.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101245