[PATCH] D102550: [ValueTracking] Mark GEP operand as nonnull if the result was loaded or stored

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

I'm somewhat concerned about the general direction here. This looks through a GEP, but what about bitcasts? What about a bitcast of a GEP? A GEP of a bitcast? A longer chain of GEPs and bitcasts? We can't reasonably walk the whole use graph and this is really pushing the bounds of what it appropriate for a ValueTracking helper.

We have a more principled version of this optimization in LVI, which scans whole blocks for pointer dereferences and records their underlying objects, thus handling this in full generality. The only caveat is that LVI only uses this information to optimize the terminator instruction, because it does not store where exactly inside the block the first dereference occurs. If the motivation here is handling of non-terminator comparisons, then that might be a better avenue to explore?



================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2135
+      const Instruction *I = cast<Instruction>(U);
+      if (DT->dominates(I, CtxI) && GEP->getType()->isPointerTy() &&
+          V == GEP->getPointerOperand() &&
----------------
How can a GEP not have pointer type?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2137
+          V == GEP->getPointerOperand() &&
+          isGEPKnownNonNull(GEP, Depth, Q, /*CheckOperand=*/false)) {
+        unsigned GEPUsesExplored = 0;
----------------
I don't get it. What is this isGEPKnownNonNull() check here for? Why do the GEP indices matter at all?


================
Comment at: llvm/lib/Analysis/ValueTracking.cpp:2138
+          isGEPKnownNonNull(GEP, Depth, Q, /*CheckOperand=*/false)) {
+        unsigned GEPUsesExplored = 0;
+        for (const auto *UGEP : GEP->users()) {
----------------
This effectively raises the maximum uses walked from DomConditionsMaxUses to DomConditionsMaxUses^2, because you can walk that many GEPs, and then that many uses of each GEP.

Shouldn't this be counting towards the main NumUsesExplored, rather than a separate limit?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102550