Page MenuHomePhabricator

[ScopedNoAliasAA] Make test basic.ll less confusing
ClosedPublic

Authored by anemet on Jan 27 2016, 7:34 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

anemet updated this revision to Diff 46213.Jan 27 2016, 7:34 PM
anemet retitled this revision from to [ScopedNoAliasAA] Make test basic.ll less confusing.
anemet updated this object.
anemet added reviewers: hfinkel, reames.
anemet added a subscriber: llvm-commits.
reames resigned from this revision.Feb 22 2016, 2:30 PM
reames removed a reviewer: reames.

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

hfinkel edited edge metadata.Mar 2 2016, 8:06 AM

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?

+Duncan

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 accepted this revision.Mar 2 2016, 11:09 AM
hfinkel edited edge metadata.

+Duncan

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.

This revision is now accepted and ready to land.Mar 2 2016, 11:09 AM
This revision was automatically updated to reflect the committed changes.