[PATCH] D71960: [Attributor] AAUndefinedBehavior: Use AAValueSimplify in memory accessing instructions.

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D71960#1875545 <https://reviews.llvm.org/D71960#1875545>, @baziotis wrote:

> In D71960#1873456 <https://reviews.llvm.org/D71960#1873456>, @jdoerfert wrote:
>
> > In D71960#1873401 <https://reviews.llvm.org/D71960#1873401>, @baziotis wrote:
> >
> > > - Added tests
> >
> >
> > All the tests have FIXMEs, right? Can you explain why they don't work yet?
>
>
> Yes. For most tests the reason is that `AAValueSimplify` doesn't give a known value. In other words, in `stopOnUndefOrAssumed()`, we don't get past the first if (`if (!ValueSimplifyAA.isKnown())`).
>
> Seeing that again though, returning from this `if` effectively means that the instruction remains assumed UB.
>  The whole procedure ends with the instruction still assumed UB. So, doesn't that fixpoint to known UB actually ?
>
> But then, check this:
>
>   define internal i32* @ret_null(i32* %A) {
>     ret i32* %A
>   }
>  
>   define void @load_null_propagated(i32* %A) {
>     %ptr = call i32* @ret_null(i32* %A)
>     %a = load i32, i32* %ptr
>     ret void
>   }
>
>
> The output is (same with AAUB disabled):
>
>   define void @load_null_propagated(i32* nocapture nofree readonly %A) #0 {
>     %a = load i32, i32* undef
>     ret void
>   }
>
>
> First of all, even if what I said above is correct, `AAValueSimplify` still gives us no value for the pointer which keeps an actually non-UB instruction to assumed till the end.
>  But then second, isn't that output wrong in general ?


I think this is a bad artifact of our current "code deletion" at the end of the Attributor run. Make the load live and it will not happen, I mean, the load in the example above can be removed either way.

  define internal i32* @ret_null(i32* %A) {
     ret i32* %A
   }
   
   define void @load_null_propagated(i32* %A) {
     %ptr = call i32* @ret_null(i32* %A)
     %a = load i32, i32* %ptr
     ret void
   }
   

I wanted to confirm and I noticed that in-tree we actually do not replace with undef, did you rebase your patch recently?

>> We also need tests that didn't work before but now, maybe just copies of the one you repaired but without the "repair" part.
> 
> Hmm, yes, that should be feasible to do in `2008-07-02-array-indexing.ll` and `PR26044.ll`. Guessing what else this patch could "repair" / improve and making new tests is more challenging.

Assuming this patch has some effect, we need to have a test for it. Then we merge and make bigger changes.


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

https://reviews.llvm.org/D71960