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

[Hal Finkel via llvm-commits]

----- Original Message -----
> From: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
> To: reviews+D16670+public+688e9466ae908760 at reviews.llvm.org
> Cc: anemet at apple.com, hfinkel at anl.gov, mssimpso at codeaurora.org, llvm-commits at lists.llvm.org
> Sent: Wednesday, March 2, 2016 2:49:46 PM
> Subject: Re: [PATCH] D16670: [ScopedNoAliasAA] Make test basic.ll less confusing
> 
> 
> > On 2016-Mar-02, at 14:09, Hal Finkel <hfinkel at anl.gov> wrote:
> > 
> > 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.
> 
> Sounds correct to me too.
> 
> Note that I do have a faint recollection of coming across weird
> canonicalization somewhere.  I think it had to do with maintaining
> the C API after the Metadata/Value split.  There is either weird
> canonicalization in LocalAsMetadata/ValueAsMetadata, or in the C
> API itself.  If you need more details I can try to find it.
> 

I also recall something like this, but it also might predate the metadata/value-decoupling work.

 -Hal

> > 
> > 
> > http://reviews.llvm.org/D16670
> > 
> > 
> > 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory