[ScopedNoAliasAA] Make test basic.ll less confusing

[anemet]

Summary

This testcase had me confused. It made me believe that you can use
alias scopes and alias scopes list interchangeably with alias.scope and
noalias. Both langref and the other testcase use scope lists so I went
looking.

Turns out using scope directly only happens to work by chance. When
ScopedNoAliasAAResult::mayAliasInScopes traverses this as a scope list:

!1 = !{!1, !0, !"some scope"}

, the first entry is in fact a scope but only because the scope is
happened to be defined self-referentially to make it unique globally.

The remaining elements in the tuple (!0, !"some scope") are considered
as scopes but AliasScopeNode::getDomain will just bail on those without
any error.

This change avoids this ambiguity in the test but I've also been
wondering if we should issue some sort of a diagnostics.

[anemet]

Ping.

[anemet]

Ping 2.

[reames]

I don't understand the alias scopes well enough to review.

[hfinkel]

I don't mind the change, but I recall that the text represents the metadata in the way that it will be canonicalized (because a metadata list of one element becomes itself?). Is that right? If so, maybe a comment would be better?

[anemet]

+Duncan

In D16670#366322, @hfinkel wrote:

I don't mind the change, but I recall that the text represents the metadata in the way that it will be canonicalized (because a metadata list of one element becomes itself?). Is that right? If so, maybe a comment would be better?

I don't know whether such canonicalization should be happening but it's certainly not happening in this case. I am also not sure how that would work. List (tuple) of metadata and the metadata itself are distinct types (MDTuple and MDNode).

For example, the function that collects the domains for an access looks like this:

void ScopedNoAliasAAResult::collectMDInDomain(
    const MDNode *List, const MDNode *Domain,
    SmallPtrSetImpl<const MDNode *> &Nodes) const {
  for (const MDOperand &MDOp : List->operands())
    if (const MDNode *MD = dyn_cast<MDNode>(MDOp))
      if (AliasScopeNode(MD).getDomain() == Domain)
        Nodes.insert(MD);
}

Where List is just the "scope" metadata node from the instruction.

In this testcase the bug is that we interpret the elements of this list (i.e. the operands of the tuple) as scopes even though these are really the attributes of the scope. I.e. in:

!1 = !{!1, !0, !"some scope"}

!1 is a scope, !0 is a domain, !"some scope" is the name of the scope.

In other words one level of indirection is missing which is what I am adding with:

!2 = !{!1}

So now the above function works as expected by seeing a single operand/element (!1) which is in fact a real scope.

I don't see how scopes or a scope-lists could be used interchangeably unless we add a way to check for it in code like the above function.

Alternatively we could introduced a new metadata type for scope lists. Then we could request scope list type on the instruction and if we encounter a scope instead, we could implicitly convert it to a scope list. I see neither of this happening so I just concluded it was not supported (and probably it shouldn't be).

Hal, Duncan, please let me know if I am missing something.

[hfinkel]

In D16670#366533, @anemet wrote:

+Duncan

In D16670#366322, @hfinkel wrote:

I don't mind the change, but I recall that the text represents the metadata in the way that it will be canonicalized (because a metadata list of one element becomes itself?). Is that right? If so, maybe a comment would be better?

I don't know whether such canonicalization should be happening but it's certainly not happening in this case. I am also not sure how that would work. List (tuple) of metadata and the metadata itself are distinct types (MDTuple and MDNode).

For example, the function that collects the domains for an access looks like this:

void ScopedNoAliasAAResult::collectMDInDomain(
    const MDNode *List, const MDNode *Domain,
    SmallPtrSetImpl<const MDNode *> &Nodes) const {
  for (const MDOperand &MDOp : List->operands())
    if (const MDNode *MD = dyn_cast<MDNode>(MDOp))
      if (AliasScopeNode(MD).getDomain() == Domain)
        Nodes.insert(MD);
}

Where List is just the "scope" metadata node from the instruction.

In this testcase the bug is that we interpret the elements of this list (i.e. the operands of the tuple) as scopes even though these are really the attributes of the scope. I.e. in:

!1 = !{!1, !0, !"some scope"}

!1 is a scope, !0 is a domain, !"some scope" is the name of the scope.

In other words one level of indirection is missing which is what I am adding with:

!2 = !{!1}

So now the above function works as expected by seeing a single operand/element (!1) which is in fact a real scope.

I don't see how scopes or a scope-lists could be used interchangeably unless we add a way to check for it in code like the above function.

Alternatively we could introduced a new metadata type for scope lists. Then we could request scope list type on the instruction and if we encounter a scope instead, we could implicitly convert it to a scope list. I see neither of this happening so I just concluded it was not supported (and probably it shouldn't be).

Hal, Duncan, please let me know if I am missing something.

Ah, indeed; sounds right. LGTM.

llvm/trunk/test/Analysis/ScopedNoAliasAA/basic.ll

 ; RUN: opt < %s -basicaa -scoped-noalias -aa-eval -evaluate-aa-metadata -print-all-alias-modref-info -disable-output 2>&1 | FileCheck %s
 ; RUN: opt < %s -aa-pipeline=basic-aa,scoped-noalias-aa -passes=aa-eval -evaluate-aa-metadata -print-all-alias-modref-info -disable-output 2>&1 | FileCheck %s
 target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @foo1(float* nocapture %a, float* nocapture readonly %c) #0 {
 entry:
 ; CHECK-LABEL: Function: foo1
-  %0 = load float, float* %c, align 4, !alias.scope !1
+  %0 = load float, float* %c, align 4, !alias.scope !2
   %arrayidx.i = getelementptr inbounds float, float* %a, i64 5
-  store float %0, float* %arrayidx.i, align 4, !noalias !1
+  store float %0, float* %arrayidx.i, align 4, !noalias !2
   %1 = load float, float* %c, align 4
   %arrayidx = getelementptr inbounds float, float* %a, i64 7
   store float %1, float* %arrayidx, align 4
   ret void
 
 ; CHECK: NoAlias:   %0 = load float, float* %c, align 4, !alias.scope !0 <->   store float %0, float* %arrayidx.i, align 4, !noalias !0
 ; CHECK: MayAlias:   %0 = load float, float* %c, align 4, !alias.scope !0 <->   store float %1, float* %arrayidx, align 4
 ; CHECK: MayAlias:   %1 = load float, float* %c, align 4 <->   store float %0, float* %arrayidx.i, align 4, !noalias !0
 ; CHECK: MayAlias:   %1 = load float, float* %c, align 4 <->   store float %1, float* %arrayidx, align 4
 ; CHECK: NoAlias:   store float %1, float* %arrayidx, align 4 <->   store float %0, float* %arrayidx.i, align 4, !noalias !0
 }
 
 attributes #0 = { nounwind uwtable }
 
 !0 = !{!0, !"some domain"}
 !1 = !{!1, !0, !"some scope"}
-
+!2 = !{!1}