[PATCH] D17080: [LAA] Allow more run-time alias checks by coercing pointer expressions to AddRecExprs

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added inline comments.


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:5
+
+; When len has a type larger than i, i can oveflow in the following kernel:
+;
----------------
"i can ove[r]flow": better clarify which i can overflow - in the test below the induction variable of the loop i is a signed 64bit (as is the bound 'len') whose bump has an nsw, so it is free of overflow concerns. The indices of A[i] and A[i+1] are (signed or unsigned?) 32bit idx and idx.inc, whose zext-add-trunc bump has no nuw nor nsw and are therefore subject to overflow concerns. Is it clear why the Added "SCEV assumptions" Flags for these should be <nssw>, rather than <nusw>? Starting from 0 and 1, nssw is more conservative.

"When len has a type": these assumptions are needed regardless of the type of len.


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:9
+;     B[i] = A[i] + A[i + 1];
+;   }
+;
----------------
Suggest to clarify the corresponding C code using both i and idx.


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:11
+;
+; If accesses to A and B can alias, we need to emmit a run-time alias check
+; between A and B. However, when i and i + 1 can wrap, their SCEV expression
----------------
"emmit" >> "emit", multiple occurrences.


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:19
+;  (sext i32 {0,+,1}<%for.body> to i64)
+;  (sext i32 {1,+,1}<%for.body> to i64)
+;
----------------
Why sext's and not zext's?


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:21
+;
+; We transformed expressions are:
+;  i64 {0,+,1}
----------------
"We" >> "The".

Hopefully the transformed expressions are
  i32 {0,+,1}
  i32 {1,+,1}
based on the added flags, right?


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:55
+
+  %idx.ext = zext i32 %idx to i64
+  %idx.ext.next = add i64 %idx.ext, 1
----------------
It doesn't really matter if we zext or sext here, right?
Or should zext indicate the type of idx was originally unsigned?


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:139
+
+; When len has a type larger than i, i can oveflow in the following kernel:
+;   for (unsigned i = 0; i < len; i++) {
----------------
See comments above.


https://reviews.llvm.org/D17080