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

[Daniel Berlin via llvm-commits]

On Mon, Jun 13, 2016 at 8:03 AM, Yichao Yu <yyc1992 at gmail.com> wrote:

> On Mon, Jun 13, 2016 at 10:35 AM, Daniel Berlin <dberlin at dberlin.org>
> wrote:
> >
> >
> > On Sun, Jun 12, 2016 at 10:12 PM, Eli Friedman <eli.friedman at gmail.com>
> > wrote:
> >>
> >> On Sun, Jun 12, 2016 at 9:31 PM, Yichao Yu <yyc1992 at gmail.com> wrote:
> >>>
> >>> > Alias analysis results should not be confused with value equivalence
> >>> > (though
> >>> > it is common).  MustAlias or NoAlias implies nothing about the actual
> >>> > pointer value, only the abstract memory location it represents.
> >>>
> >>> I can understand they are not equivalent but shouldn't not being the
> >>> same pointer value be a necessary condition for NoAlias? In another
> >>> word, are the following valid?
> >>>
> >>> ```
> >>> define i64 @f1(i64 *%p0, i64 *%p1) {
> >>>   store i64 0, i64 *%p0, !tbaa !0
> >>>   %v2 = load i64, i64 *%p1, !tbaa !1
> >>>   ret %v2
> >>> }
> >>>
> >>> define i64 @f2(i64 *%p1) {
> >>>   store i64 0, i64 *%p1, !tbaa !0
> >>>   %v2 = load i64, i64 *%p1, !tbaa !1
> >>>   ret %v2
> >>> }
> >>>
> >>
> >> It depends on what you mean by "valid"... @f1 passed two identical
> >> pointers is basically equivalent to "ret i64 undef".
> >
> >
> > So, i can't see anything in the langref that says this.
> >
> > For f1, the valid things i see you can do are (and have an argument you
> can)
> >
> > 1. move the load before the store, return value of load (IE reorder them)
> > 2. eliminate both the load and store and return 0 (IE forward the store)
> >
> > I don't see a valid path to ret undef. I'm eminently curious what i've
> > missed :)
> >
>
> So my understanding now is that this the load in question can arise
> when optimizing (pseudo code, C with llvm annotation)
>
>
> if (some_condition)
>     v = *p; !dereferencable, !tbaa !0
> else
>     v = *p; !dereferencable, !tbaa !1
>
> And when LLVM move both of the load out of the branch and merge them,
> the tbaa should be merged (and not intersect)
>

Yes, use combineMetadata, it will do the right thing.




>
>
> So my original understanding of TBAA (or other metadata that can be
> used in alias analysis) is that if two memory operations on the same
> runtime path have non intersecting tbaa metadata, they should not
> alias.


depends on what you mean by "non-intersecting".  TBAA is a set hierarchy,
so it's possible, given !1 and !2 as tbaa tags, that they do alias, if !2
is a child of !1 in the tbaa tree.


So maybe a more precise definition is that if two **used**
> memory operations on the same runtime path have non intersecting tbaa
> metadata, they should not alias. Or in another word, if a load is
> never used, then the TBAA on it doesn't count, i.e. LLVM can transform
> the code above into
>
>
This is a question of whether they are safe to speculatively execute (which
i believe they are).

> v0 = *p; !dereferencable, !tbaa !0
> v1 = *p; !dereferencable, !tbaa !1
> if (some_condition)
>     v = v0;
> else
>     v = v1;
>
> Even though the two loads are on the same runtime path, only one of
> them will ever be used so the conflicting tbaa metadata is fine?
>

It doesn't conflict in the first place.
You need to stop thinking about this as "a given pointer representing some
bits can only ever be thought of as referring to one place in memory", and
"TBAA matters at the llvm level".  TBAA is metadata. You can choose to
ignore it or not ignore it. It has no effect on semantics of the loads
themselves.

TBAA, instead,  is about language level aliasing semantics.  It is saying
"even though these loads/stores look the same, they are not to the same
memory location" or "even though these loads/stores  look not the same,
they are the same memory location".

It is metadata. You can choose to ignore it or not ignore it, at your
leisure. It changes *no semantics*.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160613/78d2f0dd/attachment.html>