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

[Bardia Mahjour via Phabricator via llvm-commits]

bmahjour added inline comments.


================
Comment at: llvm/include/llvm/Analysis/DependenceAnalysis.h:285
     /// FullDependence) with as much information as can be gleaned.
-    /// The flag PossiblyLoopIndependent should be set by the caller
-    /// if it appears that control flow can reach from Src to Dst
-    /// without traversing a loop back edge.
+    /// If PossiblyLoopIndependant is true will also test for intra-iteration
+    /// dependencies.
----------------
Meinersbur wrote:
> Maybe we should rename `PossiblyLoopIndependent` to something else. I don't see how "LoopIndependent" makes a statement about non-loop dependences.
Any suggestions on what the rename should be? "Loop-Independent" is well defined in the literature and seems appropriately used here.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3560
+  if (!PossiblyLoopIndependent && CommonLevels == 0)
+    return nullptr;
+
----------------
add a `LLVM_DEBUG` before returning?


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3571-3574
+  // 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.
----------------
Meinersbur wrote:
> `underlyingObjectsAlias` should return `PartialAlias` alias in such cases. That is, I think it still may return `MustAlias` if the address is the same but the size is different. In that case it should be sufficient to a compare just the sizes, but not the alignment
I agree this logic belongs to `underlyingObjectsAlias`.


================
Comment at: llvm/lib/Analysis/DependenceAnalysis.cpp:3575
+  // Reject these cases.
+  auto IsUnalignedLoadStore = [](const DataLayout *DL, const Instruction *I) {
+    const LoadInst *LI = dyn_cast<LoadInst>(I);
----------------
DL doesn't change in any invocation of the lambda, so better be captured instead of being passed as arg.


================
Comment at: llvm/test/Analysis/DependenceAnalysis/OverlappingAddresses.ll:23
+
+  %add1 = add i64 %n, 1
+  %arrayidx2 = getelementptr inbounds i32, i32* %A, i64 %add1
----------------
Should we also check if we can prove that the pointer differences will always be larger than the misalignment? For example if this was adding 2 to the base pointer, then the store on line 14 would not overlap with the two loads below.


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

https://reviews.llvm.org/D108048