[PATCH] D28126: [MemDep] Handle gep with zeros for invariant.group

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

While I think this patch is simple and a nice pragmatic improvement, I don't want to prematurely cut off discussion about whether we have the right canonicalization here. Maybe leave a FIXME about the fact that we have two representations that are used? Unsure

I suspect something like this should go in at least while instcombine is actively canonicalizing in this direction just for practical reasons.



================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:354-355
   // Queue to process all pointers that are equivalent to load operand.
-  SmallVector<Value *, 8> LoadOperandsQueue;
-  LoadOperandsQueue.push_back(LoadOperand);
-  Seen.insert(LoadOperand);
+  SmallVector<const Value *, 8> LoadOperandsQueue;
+  SmallSet<const Value *, 14> SeenValues;
+  auto TryInsertToQueue = [&LoadOperandsQueue, &SeenValues](Value *V) {
----------------
Is the adding of const everywhere helping much? We've historically avoided it, but I have no really strong opinions...


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:356-359
+  auto TryInsertToQueue = [&LoadOperandsQueue, &SeenValues](Value *V) {
+    if (SeenValues.insert(V).second)
+      LoadOperandsQueue.push_back(V);
+  };
----------------
I think its fine to just use [&] capture on simple factorings like this.


================
Comment at: lib/Analysis/MemoryDependenceAnalysis.cpp:383-384
+      // U = bitcast Ptr  ||  U = getelementptr Ptr, 0, 0...
+      auto *BCI = dyn_cast<BitCastInst>(U);
+      auto *GEP = dyn_cast<GetElementPtrInst>(U);
+      if (BCI || (GEP && GEP->hasAllZeroIndices())) {
----------------
It would be nice to avoid going through the type lookup machinery twice here.


https://reviews.llvm.org/D28126