[PATCH] D91576: [MemCpyOpt] Bail on call slot optimization if it merges alias scopes

[Di Mo via Phabricator via llvm-commits]

modimo added a comment.

I've looked more into what alias.scopes are and I think I've got a better handle on what's actually wrong. In the meantime let me try to describe the original problem better. The first section is checking my understanding and making sure we're on the same page. Skip to the second part for the bug details.

My understanding of domains
---------------------------

Let's say we have the following (I slightly modified test/Transforms/Inline/noalias.ll for this:

  define void @hello(float* noalias nocapture %a, float* nocapture readonly %c) #0 {
  entry:
    %0 = load float, float* %c, align 4
    %arrayidx = getelementptr inbounds float, float* %a, i64 5
    store float %0, float* %arrayidx, align 4
    ret void
  }
  
  define void @hello2(float* noalias nocapture %a, float* nocapture readonly %c) #0 {
  entry:
    %0 = load float, float* %c, align 4
    %arrayidx = getelementptr inbounds float, float* %a, i64 5
    store float %0, float* %arrayidx, align 4
    ret void
  }
  
  define void @foo(float* nocapture %a, float* nocapture readonly %c) #0 {
  entry:
    tail call void @hello(float* %a, float* %c)
    tail call void @hello2(float* %a, float* %c)
    %0 = load float, float* %c, align 4
    %arrayidx = getelementptr inbounds float, float* %a, i64 7
    store float %0, float* %arrayidx, align 4
    ret void
  }

After `-inline -enable-noalias-to-md-conversion`

  ; Function Attrs: nounwind uwtable
  define void @foo(float* nocapture %a, float* nocapture readonly %c) #0 {
  entry:
    %0 = load float, float* %c, align 4, !noalias !0
    %arrayidx.i = getelementptr inbounds float, float* %a, i64 5
    store float %0, float* %arrayidx.i, align 4, !alias.scope !0
    %1 = load float, float* %c, align 4, !noalias !3
    %arrayidx.i1 = getelementptr inbounds float, float* %a, i64 5
    store float %1, float* %arrayidx.i1, align 4, !alias.scope !3
    %2 = load float, float* %c, align 4
    %arrayidx = getelementptr inbounds float, float* %a, i64 7
    store float %2, float* %arrayidx, align 4
    ret void
  }
  
  attributes #0 = { nounwind uwtable }
  
  !0 = !{!1}
  !1 = distinct !{!1, !2, !"hello: %a"}
  !2 = distinct !{!2, !"hello"}
  !3 = !{!4}
  !4 = distinct !{!4, !5, !"hello2: %a"}
  !5 = distinct !{!5, !"hello2"}

Looking at the code in `AddAliasScopeMetadata` I believe what happens is that:

1. We create a new domain (!2 and !5) for each inline site. Doesn't matter if the callee is the same (say if I called hello twice) two still get created
2. Scopes are created around arguments that are marked `noalias`, so in this case !4 and !1
3. Alias sets are the last to be created, these are what's tagged on the actual instructions themselves (!0 and !3)

When querying AA results by adding `-basic-aa -scoped-noalias-aa -aa-eval -evaluate-aa-metadata -print-all-alias-modref-info` to opt on this pair:

  MayAlias:   %0 = load float, float* %c, align 4, !noalias !0 <->   store float %1, float* %arrayidx.i1, align 4, !alias.scope !3

The following steps are taken:

1. Find the underlying domains left to right behind !noalias !0, in this case !2 but it could be more
2. Find all scopes in in these domains that belong to !0 (noalias_set) and !3 (aliasscope_set)
  1. In this case, noalias_set={!1} and aliasscope_set={}
3. Since aliasscope_set is empty here that's a bail condition and we conclude MayAlias

In a more interesting case such as:

  NoAlias:   %0 = load float, float* %c, align 4, !noalias !0 <->   store float %0, float* %arrayidx.i, align 4, !alias.scope !0

Here noalias_set={!1} and aliasscope_set={!1} and we check to see if noalias_set is a superset of aliasscope_set. Since it is, we can concluded this is noalias

In the step (2) above though this is isolated to a single domain at a time. When for any single domain the superset relationship holds, then we immediately conclude noalias.
I think this is because domains are nested based from inlining. If I have domains {!1, !2} in my metadata, that indicates the I'm currently in callee2 of this call chain caller->callee1->callee2. Thus, as long as this property holds the first domain that matches with a superset is enough to conclude noalias.

How the bug I'm looking at manifests
------------------------------------

In memcpyopt we can fold 2 memcpys into a single one. This causes the alias.scope metadata to be merged in a union. Suppose I'm merging the following 2 instances (I'm reusing the metadata from the above examples):

  define i8 @test(i8 %input) {
    %tmp = alloca i8
    %dst = alloca i8
    %src = alloca i8
    call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %src), !noalias !3
    store i8 %input, i8* %src
    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %tmp, i8* align 8 %src, i64 1, i1 false), !alias.scope !1
    call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %src), !noalias !3
    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %tmp, i64 1, i1 false), !alias.scope !3
    %ret_value = load i8, i8* %dst
    ret i8 %ret_value
  }
  
  declare void @llvm.lifetime.start.p0i8(i64, i8* nocapture)
  declare void @llvm.lifetime.end.p0i8(i64, i8* nocapture)
  declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i1)
  
  !0 = !{!1}
  !1 = distinct !{!1, !2, !"hello: %a"}
  !2 = distinct !{!2, !"hello"}
  !3 = !{!4}
  !4 = distinct !{!4, !5, !"hello2: %a"}
  !5 = distinct !{!5, !"hello2"}

The merged instance then will look like (`-memcpyopt -enable-noalias-to-md-conversion -basic-aa -scoped-noalias-aa -aa-eval -evaluate-aa-metadata -print-all-alias-modref-info -S`)

  define i8 @test(i8 %input) {
    %tmp = alloca i8, align 1
    %dst = alloca i8, align 1
    %src = alloca i8, align 1
    call void @llvm.lifetime.start.p0i8(i64 8, i8* nonnull %src), !noalias !0
    store i8 %input, i8* %src, align 1
    call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %src, i64 1, i1 false), !alias.scope !3
    call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %src), !noalias !0
    %ret_value = load i8, i8* %dst, align 1
    ret i8 %ret_value
  }
  ...
  !0 = !{!1}
  !1 = distinct !{!1, !2, !"hello2: %a"}
  !2 = distinct !{!2, !"hello2"}
  !3 = !{!1, !4, !5, !"hello: %a"}
  !4 = distinct !{!4, !5, !"hello: %a"}
  !5 = distinct !{!5, !"hello"}

!3 now represents the union of the previous scopes. Now, if I ask AA what the relationship between:

  NoModRef:   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %src, i64 1, i1 false), !alias.scope !3 <->   call void @llvm.lifetime.end.p0i8(i64 8, i8* nonnull %src), !noalias !0

I get a result which seems incorrect. lifetime.end definitely does ref %src from the previous memcpy. When querying scoped-AA we look at the first domain inside `!noalias !0`, which is !2 and its scopes which is {!1}. The scopes in that domain for `!alias.scope !3` is however also only {!1} as well since the added metadata is in a separate domain. Thus because {!1} is a superset of {!1} scoped-AA concludes its nomodref.

I think the underlying problem is that scoped-AA relies on any appended domains to reside in a caller-callee dependence. If that holds, in this example you wouldn't care that you have another domain in `!alias.scope !3` since it would be a callee within a scope that we know doesn't alias. However, because memcpyopt is not inlining and doesn't preserve this rule the merged metadata doesn't express the new dependence correctly and we lose aliasing information.


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

https://reviews.llvm.org/D91576