[PATCH] D21271: Fix `InstCombine` to not widen metadata on store-to-load forwarding

[Yichao Yu via llvm-commits]

On Mon, Jun 13, 2016 at 12:04 AM, Yichao Yu <yyc1992 at gmail.com> wrote:
> On Sun, Jun 12, 2016 at 11:48 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>>
>> On Sun, Jun 12, 2016 at 8:03 PM, Yichao Yu via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>>
>>> yuyichao added a comment.
>>>
>>> In another word, IMHO,
>>>
>>>   %v1 = load i64, i64 *%p1, !tbaa !0
>>>   %v2 = load i64, i64 *%p1, !tbaa !1
>>>
>>> should be invalid IR (and the optimizer should not be allowed to create
>>> this) if `!1` and `!2` are non-aliasing tbaa node.
>>
>> Why?
>> It's entirely possible to create a language where this is a valid and useful
>> lowering.
>> TBAA applies *to the load*, not to the *pointer*, and the langref says
>> nothing about it requiring it to assign consistent TBAA nodes to different
>> loads of the same pointer.
>>
>
> My understanding is that the tbaa metadata is used for alias analysis.
> What should be the result of the alias analysis on the two loads in
> this case? Won't
> tbaa return no aliasing while some other returns always aliasing for
> this case?


On Mon, Jun 13, 2016 at 12:04 AM, Yichao Yu <yyc1992 at gmail.com> wrote:
> On Sun, Jun 12, 2016 at 11:48 PM, Daniel Berlin <dberlin at dberlin.org> wrote:
>>
>>
>> On Sun, Jun 12, 2016 at 8:03 PM, Yichao Yu via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>>>
>>> yuyichao added a comment.
>>>
>>> In another word, IMHO,
>>>
>>>   %v1 = load i64, i64 *%p1, !tbaa !0
>>>   %v2 = load i64, i64 *%p1, !tbaa !1
>>>
>>> should be invalid IR (and the optimizer should not be allowed to create
>>> this) if `!1` and `!2` are non-aliasing tbaa node.
>>
>> Why?
>> It's entirely possible to create a language where this is a valid and useful
>> lowering.
>> TBAA applies *to the load*, not to the *pointer*, and the langref says
>> nothing about it requiring it to assign consistent TBAA nodes to different
>> loads of the same pointer.
>>
>
> My understanding is that the tbaa metadata is used for alias analysis.
> What should be the result of the alias analysis on the two loads in
> this case? Won't
> tbaa return no aliasing while some other returns always aliasing for
> this case?

It's probably a bad example since there's no store, a better one might be

```
store i64 %v0, i64 *%p1, !tbaa !1
%v1 = load i64, i64 *%p1, !tbaa !0
%v2 = load i64, i64 *%p1, !tbaa !1
```

or

```
store i64 %v0, i64 *%p0, !tbaa !1
%v1 = load i64, i64 *%p1, !tbaa !0
%v2 = load i64, i64 *%p1, !tbaa !1
```

where `%p0` and `%p1` actually can alias at runtime. What I'm saying
is basically that if the user code is

```
store i64 %v0, i64 *%p0, !tbaa !1
%v2 = load i64, i64 *%p1, !tbaa !1
```

The optimizer shouldn't be allowed to insert a `%v1 = load i64, i64
*%p1, !tbaa !0`. If it is moved from a dead branch and the optimizer
can't prove that the metadata is valid, the compiler should make it

```
store i64 %v0, i64 *%p0, !tbaa !1
%v1 = load i64, i64 *%p1
%v2 = load i64, i64 *%p1, !tbaa !1
```

instead and then load CSE can make this

```
store i64 %v0, i64 *%p0, !tbaa !1
%v1 = load i64, i64 *%p1, !tbaa !1
```

with all use of %v2 replaced by %v1.

IIUC, The commit message of r241886 claims that having incompatible
metadata can cause mis-optimization and I expect such cases to involve
memory store too since otherwise there's no need to perform alias
analysis.