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

[Stefanos Baziotis via Phabricator via llvm-commits]

baziotis added a comment.

In D71960#1875557 <https://reviews.llvm.org/D71960#1875557>, @jdoerfert wrote:

> 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.


You mean because it's unused anyway? Indeed if we return `%a` for example, it's not replaced with undef.

>   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?

Not too long ago, about 3-4 days ago but now that I rebased again, it seems it acts differently. I'll update the diff.

> 
> 
>>> 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.

Yes, I think new tests test the new things. I'll update the repaired as well and then we see if it needs anything more.


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

https://reviews.llvm.org/D71960