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

[silviu.baranga@arm.com via Phabricator via llvm-commits]

sbaranga 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:
+;
----------------
Ayal wrote:
> "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.
Thanks for the observations. There is a discrepancy here between the C code and the IR being tested (and the text is misleading). We are actually looking at the expressions for %inc.ptr0 and %inc.ptr1.

The sign extension comes from the getelementptr instruction. Because the SCEV expression that we are trying to linearize is something like (sext i32 {0,+,1}<%for.body> to i64), we can only add the nssw flag. 

I'll update the text to clarify things.


================
Comment at: test/Analysis/LoopAccessAnalysis/memcheck-wrapping-pointers.ll:21
+;
+; We transformed expressions are:
+;  i64 {0,+,1}
----------------
Ayal wrote:
> "We" >> "The".
> 
> Hopefully the transformed expressions are
>   i32 {0,+,1}
>   i32 {1,+,1}
> based on the added flags, right?
i64 should be correct. We're essentially sinking the sext to get a i64 linear expression.


================
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
----------------
Ayal wrote:
> It doesn't really matter if we zext or sext here, right?
> Or should zext indicate the type of idx was originally unsigned?
Correct (for both). 


https://reviews.llvm.org/D17080