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

[Duncan P. N. Exon Smith]

> 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.