[LLVMdev] [RFC] Separating Metadata from the Value hierarchy

[David Blaikie]

On Mon, Nov 10, 2014 at 9:08 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

>
> > On 2014-Nov-09, at 22:24, Hal Finkel <hfinkel at anl.gov> wrote:
> >
> > ----- Original Message -----
> >> From: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
> >> To: "LLVM Developers Mailing List" <llvmdev at cs.uiuc.edu>
> >> Sent: Sunday, November 9, 2014 7:02:43 PM
> >> Subject: [LLVMdev] [RFC] Separating Metadata from the Value hierarchy
> >>
> >>
> >>
> >> TL;DR: If you use metadata (especially if it's out-of-tree), check
> >> the
> >> numbered list of lost functionality below to see whether I'm trying
> >> to
> >> break your compiler permanently.
> >>
> >> In response to my recent commits (e.g., [1]) that changed API from
> >> `MDNode` to `Value`, Eric had a really interesting idea [2] -- split
> >> metadata entirely from the `Value` hierarchy, and drop general
> >> support
> >> for metadata RAUW.
> >>
> >> After hacking together some example code, this seems overwhelmingly
> >> to
> >> me like the right direction. See the attached metadata-v2.patch for
> >> my
> >> sketch of what the current metadata primitives might look like in the
> >> new hierarchy (includes LLVMContextImpl uniquing support).
> >>
> >> The initial motivation was to avoid the API weaknesses caused by
> >> having
> >> non-`MDNode` metadata that inherits from `Value`. In particular,
> >> instead of changing API from `MDNode` to `Value`, change it to a base
> >> class called `Metadata`, which sheds the underused and expensive
> >> `Value`
> >> base class entirely.
> >>
> >> The implications are broader: an enormous reduction in complexity
> >> (and
> >> overhead) for metadata.
> >>
> >> This change implies minor (major?) loss of functionality for
> >> metadata,
> >> but Eric and I suspect that the only hard-to-fix user is debug info
> >> itself, whose IR infrastructure I'm rewriting anyway.
> >>
> >> Here is what we'd lose:
> >>
> >> 1. No more arbitrary RAUW of metadata.
> >>
> >> While we'd keep support for RAUW of temporary MDNodes for use as
> >> forward declarations (when parsing assembly or constructing cyclic
> >> graphs), drop RAUW support for all other metadata.
> >
> > So temporary MDNodes would be Values, but regular metadata would not be?
> Will regular metadata nodes no longer have lists of users? If I have a
> TrackingVH<MDNode> with temporary MDNodes, after I call replaceAllUsesWith,
> what happens?
> >
> > I'm specifically wondering how we'll need to update
> CloneAliasScopeMetadata in lib/Transforms/Utils/InlineFunction.cpp, and I
> don't see any fundamental problem with what you've proposed, but these seem
> like generic upgrade questions.
> >
> > Thanks again,
> > Hal
>
> So, none of them would be `Value`s.
>
> `TempMDNode` supports its own, non-`Value`-based, RAUW via
> `ReplaceableMetadataImpl` (`ValueAsMetadata` uses the same).
>
> `CloneAliasScopeMetadata()` should use `TrackingMDRef` in place of
> `TrackingVH<MDNode>`.  `TrackingMDRef` will register itself with the
> `Metadata` if it's replaceable, and if/when it gets RAUW'ed, the pointer
> will get updated.
>
> If you have another look at the patch it might be more clear now.
>
> BTW, another thing I added in that sketch was the option to explicitly
> request a non-uniqued `MDNode`.  Haven't thought through details, but I
> was specifically thinking this would be a cleaner way to create your
> non-uniquable alias nodes (instead of the current self-referential
> approach).
>
> My working straw man for assembly syntax looks like this:
>
>     !1 = metadata distinct !{ ... }
>
> As an example, the alias "root" nodes could change from this:
>
>     !1 = metadata !{ metadata !1 }
>
> to this:
>
>     !1 = metadata distinct !{}
>
> Which means constructing them would change from this:
>
>     MDNode *T = MDNode::getTemporary(Context, None)
>     MDNode *N = MDNode::get(Context, {T});
>     T->replaceAllUsesWith(N);
>     MDNode::deleteTemporary(T);
>
> to this:
>
>     // In that patch it's "getNonUniqued()".
>     MDNode *N = MDNode::getDistinct(Context, None);
>
> Furthermore, if all references to the alias root got dropped, they'd
> clean themselves up instead of keeping each other alive in a cycle.
>

There are at least two bugs I know of in debug info that are due to
overzealous metadata uniqing:

1) inline two calls to the same function on the same line and we end up
with one inlined call (inlined location + original location are the same,
so the scope becomes one scope instead of two). We workaround this in the
frontend by adding column info to calls (but we don't do it for constructor
calls or member function calls... ) but any workaround is insufficient as,
in the limit, you can put the two calls in a macro and then they'll be
attributed to the same line and column. (I cc'd you on a bug about this a
week or two ago)

2) two anonymous structs in the same location (again, even adding column
info doesn't save you from a macro) end up as one struct... (which gets
weird with duplicate members, etc)

There could easily be others, too.


>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20141110/ef562cc5/attachment.html>