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

[Piotr Padlewski via Phabricator via llvm-commits]

Prazek added inline comments.


================
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) {
----------------
chandlerc wrote:
> Is the adding of const everywhere helping much? We've historically avoided it, but I have no really strong opinions...
I only added 4 consts, where 2 are inside container and 2 in the loop (which could be avoided by using auto, but in that cases it seems cleaner without auto). 

I prefer const in the container types because I can show this way that I am not going to modify any of these values


================
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())) {
----------------
chandlerc wrote:
> It would be nice to avoid going through the type lookup machinery twice here.
Code elegance your concern here right?
I can put it in ifs like

  if (isa<BitCastInst>(U)) {
    TryInsertToQueue(U);
    continue;
  }
  if (auto *GEP = dyn_cast<GetElementPtr>(U)) {
    if(GEP->hasAllZeroIndices())
      TryInsertToQueue(U);
    continue;
  }

I am not sure if it will be cleaner (specially because of nested if)

  
  


https://reviews.llvm.org/D28126