[PATCH] D16670: [ScopedNoAliasAA] Make test basic.ll less confusing

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 2 11:09:03 PST 2016


hfinkel accepted this revision.
hfinkel added a comment.
This revision is now accepted and ready to land.

In http://reviews.llvm.org/D16670#366533, @anemet wrote:

> +Duncan
>
> In http://reviews.llvm.org/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.


http://reviews.llvm.org/D16670





More information about the llvm-commits mailing list