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

[Vitaly Buka via Phabricator via llvm-commits]

vitalybuka 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:
> 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);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84906