[PATCH] D84906: [NFC] Use GetUnderlyingObjects in findAllocaForValue

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:4302-4307
 AllocaInst *llvm::findAllocaForValue(Value *V) {
-  DenseMap<Value *, AllocaInst *> AllocaForValue;
-  return ::findAllocaForValue(V, AllocaForValue);
+  SmallVector<const Value *, 16> Objects;
+  GetUnderlyingObjects(V, Objects);
+  if (Objects.size() != 1)
+    return nullptr;
+  return const_cast<AllocaInst *>(dyn_cast<AllocaInst>(Objects.front()));
----------------
jdoerfert wrote:
> vitalybuka wrote:
> > vitalybuka wrote:
> > > jdoerfert wrote:
> > > > It seems silly that we have the single object and multi object interface that do different things. Arguably, you would like to write the above code. Maybe we should allow `GetUnderlyingObject` to (optionally) look through PHI and select instead? Making that happen seems little work but makes these awkward invocations unnecessary. WDYT?
> > > To handle select and PHI we need search like in getUnderlyingObjects, which requires Visited and Worklist on the stack.
> > > So we will have add similar silly code into optional part of getUnderlyingObject.
> > > Even if this part is OK, adding new option into public interface looks like adding more complexity then we trying to remove here from non-public part.
> > > Ideally we would like to support select and PHI non-optionally, but this requires some validation of all call sites which is not a trivial piece of work.
> > > 
> > > Maybe I can do the following.
> > > This is more efficient but it does not look better to me.
> > > 
> > > ```
> > > template <class F>
> > > static void findUnderlyingObjects(const Value *V, F OnObj, LoopInfo *LI = nullptr,
> > >                                   unsigned MaxLookup = 6) {
> > >   SmallPtrSet<const Value *, 4> Visited;
> > >   SmallVector<const Value *, 4> Worklist;
> > >   Worklist.push_back(V);
> > >   do {
> > >     const Value *P = Worklist.pop_back_val();
> > >     P = getUnderlyingObject(P, MaxLookup);
> > > 
> > >     if (!Visited.insert(P).second)
> > >       continue;
> > > 
> > >     if (auto *SI = dyn_cast<SelectInst>(P)) {
> > >       Worklist.push_back(SI->getTrueValue());
> > >       Worklist.push_back(SI->getFalseValue());
> > >       continue;
> > >     }
> > > 
> > >     if (auto *PN = dyn_cast<PHINode>(P)) {
> > >       // If this PHI changes the underlying object in every iteration of the
> > >       // loop, don't look through it.  Consider:
> > >       //   int **A;
> > >       //   for (i) {
> > >       //     Prev = Curr;     // Prev = PHI (Prev_0, Curr)
> > >       //     Curr = A[i];
> > >       //     *Prev, *Curr;
> > >       //
> > >       // Prev is tracking Curr one iteration behind so they refer to different
> > >       // underlying objects.
> > >       if (!LI || !LI->isLoopHeader(PN->getParent()) ||
> > >           isSameUnderlyingObjectInLoop(PN, LI))
> > >         for (Value *IncValue : PN->incoming_values())
> > >           Worklist.push_back(IncValue);
> > >       continue;
> > >     }
> > > 
> > >     if (!OnObj(P))
> > >       return;
> > >   } while (!Worklist.empty());
> > > }
> > > 
> > > void llvm::getUnderlyingObjects(const Value *V,
> > >                                 SmallVectorImpl<const Value *> &Objects,
> > >                                 LoopInfo *LI, unsigned MaxLookup) {
> > >   findUnderlyingObjects(
> > >       V,
> > >       [&Objects](const Value *V) {
> > >         Objects.push_back(V);
> > >         return true;
> > >       },
> > >       LI, MaxLookup);
> > > }
> > > 
> > > AllocaInst *llvm::findAllocaForValue(Value *V) {
> > >   const AllocaInst *A = nullptr;
> > >   findUnderlyingObjects(V, [&A](const Value *V) -> bool {
> > >     if (!A)
> > >       A = dyn_cast<AllocaInst>(V);
> > >     else if (A != V)
> > >       A = nullptr;
> > >     return A;
> > >   });
> > >   return const_cast<AllocaInst *>(A);
> > > }
> > > ```
> > Actually OnObj can't be called twice for the same obj, so
> > ```
> > AllocaInst *llvm::findAllocaForValue(Value *V) {
> >   const AllocaInst *A = nullptr;
> >   findUnderlyingObjects(V, [&A](const Value *V) -> bool {
> >      A = A ? nullptr : dyn_cast<AllocaInst>(V);
> >      return A;
> >   });
> >   return const_cast<AllocaInst *>(A);
> > }
> > ```
> My main point was that we should merge the `object` and `objects` callbacks such that in the former you stop at the second object while in the latter you don't. Basically what you did for the findAlloca case now, except that I think this code should be the behavior of the object API. FindAlloca is then a call and a dyn_cast_or_null. That said, there might be compile time concerns which might justify a flag on the objects API that directly calls what is now `getUnderlyingObject`. I'm not in favor of the flag FWIW.
> 
> Maybe others want to chime in, to name a few @lebedev.ri @efriedma @spatel 
Making getUnderlyingObject() as powerful as getUnderlyingObjects() makes sense to me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84906