[PATCH] D108048: [DependenceAnalysis] Conservatively exit on type mismatch

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added inline comments.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3595-3598
+  // DependenceAnalysis isn't able to deal with different
+  // access widths. If access widths are the same, but alignment is
+  // smaller than the store size, accesses could overlap.
+  // Reject these cases.
----------------
Seems to make sense that DependenceAnalysis does not handle accesses that fall between two array elements. Am I correct that his makes sure that each load/store will access at most one alignment "slot"? And therefore conservatively bails out for any type that is larger than its alignment (such as _Complex, struct, classes, etc.)?

In that case, I propose a reformulation:
> DependenceAnalysis assumes that accesses never partially overlap. Here we ensure that every access size is smaller than the alignment. This guarantees that two aligned accesses either have the same pointer value, or do not alias, not even partially.
> FIXME: This excludes many common cases from analysis, such as array of structs, _Complex and bitcasts to i8* (eg. memset, memcpy, fallback type for many optimization passes).




================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3599
+  // Reject these cases.
+  auto IsUnalignedLoadStore = [&](const Instruction *I) {
+    const LoadInst *LI = dyn_cast<LoadInst>(I);
----------------
An "unaligned access" would be an access that is ... not aligned. However, this lambda tests whether partial overlap is possible despite alignment. Proposed name: "IsAccSizeExceeedingAlignment" (or some of the like).


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3606
+
+    return ((Alignment % Width) != 0);
+  };
----------------
[nit] redundant outer parens

Is `Width <= Alignment` sufficient?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3609-3610
+
+  bool ModifiedGEPOp =
+      !dyn_cast<GEPOperator>(SrcPtr) || !dyn_cast<GEPOperator>(DstPtr);
+  Type *SrcOpType =
----------------
What makes partial overlap impossible if both pointer's last operation is a GEP?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3611-3616
+  Type *SrcOpType =
+      static_cast<PointerType *>(MemoryLocation::get(Src).Ptr->getType())
+          ->getElementType();
+  Type *DstOpType =
+      static_cast<PointerType *>(MemoryLocation::get(Dst).Ptr->getType())
+          ->getElementType();
----------------
`llvm::Type::getPointerElementType()` is a shortcut for `cast<PointerType>(...)->getElementType()`


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

https://reviews.llvm.org/D108048