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

[Hal Finkel]

----- Original Message -----
> From: "Duncan P. N. Exon Smith" <dexonsmith at apple.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "LLVM Developers Mailing List" <llvmdev at cs.uiuc.edu>
> Sent: Monday, November 10, 2014 11:08:24 AM
> Subject: Re: [LLVMdev] [RFC] Separating Metadata from the Value hierarchy
> 
> 
> > 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.

Yes, thanks!

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

This sounds like a great improvement!

 -Hal

> 

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