[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()));
----------------
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);
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84906