[PATCH] D154714: LAA: handle GEPs with >2 operands in findForkedSCEVs()

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Not familiar with this code, but this doesn't look correct to me.



================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:866
+    for (unsigned i = 1; i < I->getNumOperands(); ++i)
+      findForkedSCEVs(SE, L, I->getOperand(i), OffsetScevs, Depth);
 
----------------
This seems to assume that the same scale can be applied to all GEP indices, which is not correct. I think the only reason this happens to work for your example is that one of the indices is zero.

If you want to handle this generically, what you should probably do is either a) use collectOffsets() to convert the GEP into scale multiply representation or b) get the SCEV for the GEP and analyze SCEVUnknowns for forking.


================
Comment at: llvm/lib/Analysis/LoopAccessAnalysis.cpp:954
+      ScevList.emplace_back(Scev, !isGuaranteedNotToBeUndefOrPoison(Ptr));
+    break;
+  }
----------------
If I'm reading this code right, this is claiming that `load ptr, ptr %p` is the same as `%p`? That doesn't make sense. `load` should always be a root instruction, as it was before.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D154714