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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added subscribers: efriedma, spatel, lebedev.ri.
jdoerfert 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()));
----------------
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 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84906