[PATCH] D62939: Fix a bug w/inbounds invalidation in LFTR (was: Strengthen LFTR's ability to replace IVs)

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D62939#1538984 <https://reviews.llvm.org/D62939#1538984>, @reames wrote:

> Actually, think 'S' being poisoned is almost fully handled or maybe even fully handled.  If 'S' is poison, then IV must be poison on at least the first iteration. Given that, if we find a side effect which must execute on the path to our exit block, then we've proven we must have executed UB.  As such, the program is undefined and we have no further obligations.
>
> The only case where this line of reasoning doesn't work is a) our exit itself isn't evaluated on the first iteration and b) there's some operation within the IV computation which 1) strips poison from a poison input and 2) contributes to the next iteration of the IV.
>
> Do there even exist operations which perform (b)?  That would seem to be a contradiction.
>
> If there aren't, then we're fully covered.  If there are, then we'd need to find a UB operation on the path to the backedge to ensure poison on a prefix of the iteration space (only) is covered.  This would be easy to implement, though I'd prefer to do so in a separate change for sanity at this point.  :)


If the mustExecuteUBIfPoisonOnPathTo() check is performed, then, as you argue, we do handle `S` being poison properly. It's just that the check is only performed on pointers, but not integers (in this patch).

In any case, let's defer that one, this patch is about pointers. There's one last tweak I'd like to see here: If the condition already depends on the post-inc IV, then we can safely use it. Going through the test cases this is actually the case for most of the regressions in existing tests (I've left comments), so I think it would be very worthwhile to check this case.



================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2080
+/// Return true if undefined behavior would provable be executed on the path to
+/// OnPathTo if Root produced a posion result.  Note that this doesn't say
+/// anything about whether OnPathTo is actually executed or whether Root is
----------------
posion -> poison


================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:2092
+  // users are provable UB and must execute before out exiting block might
+  // exit.
+
----------------
Given the now more general formulation of the function, the mention of "exiting block" should probably be dropped.


================
Comment at: test/Transforms/IndVarSimplify/2011-11-01-lftrptr.ll:345
   %incdec.ptr1608 = getelementptr i8, i8* %r.17193, i64 1
   %cmp1604 = icmp ult i8* %incdec.ptr1608, %add.ptr1603
   br i1 %cmp1604, label %for.body, label %for.end1609
----------------
Condition already depends on post-inc IV.


================
Comment at: test/Transforms/IndVarSimplify/lftr-address-space-pointers.ll:47
   %incdec.ptr = getelementptr inbounds i8, i8 addrspace(2)* %p.02, i32 1
   %cmp = icmp ult i8 addrspace(2)* %incdec.ptr, %add.ptr
   br i1 %cmp, label %for.body, label %for.end
----------------
Condition already depends on post-inc IV.


================
Comment at: test/Transforms/IndVarSimplify/lftr-address-space-pointers.ll:97
   %incdec.ptr = getelementptr inbounds i8, i8 addrspace(3)* %p.02, i32 1
   %cmp = icmp ult i8 addrspace(3)* %incdec.ptr, %add.ptr
   br i1 %cmp, label %for.body, label %for.end
----------------
Condition already depends on post-inc IV.


================
Comment at: test/Transforms/IndVarSimplify/lftr-reuse.ll:56
   %incdec.ptr = getelementptr inbounds i8, i8* %p.02, i32 1
   %cmp = icmp ult i8* %incdec.ptr, %add.ptr
   br i1 %cmp, label %for.body, label %for.end
----------------
Condition already depends on post-inc IV.


================
Comment at: test/Transforms/IndVarSimplify/lftr-reuse.ll:331
   store i8 3, i8* %aptr
   %cmp = icmp ult i8* %incdec.ptr, %ivend
   br i1 %cmp, label %loop, label %exit
----------------
Condition already depends on post-inc IV.


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

https://reviews.llvm.org/D62939