[LLVMdev] [proposal] Extensible IR metadata

[Jeffrey Yasskin]

On Tue, Sep 15, 2009 at 11:37 PM, Devang Patel <devang.patel at gmail.com> wrote:
> On Fri, Sep 11, 2009 at 9:57 AM, Chris Lattner <clattner at apple.com> wrote:
>> Devang's work on debug info prompted this, thoughts welcome:
>> http://nondot.org/sabre/LLVMNotes/ExtensibleMetadata.txt
>
> Here is partial initial implementation.
> Thoughts ?

setHasMetadata probably shouldn't be public, like there's no way to
directly set HasValueHandle.

I'd make the MDKindId type (or typedef) now so it's obvious what those
"unsigned"s mean.

Should the metadata name->id map be per-context or global? I think
it's unlikely people will be registering these things during
compilation, so we can afford the overhead of a lock, and making it
global helps with the wrapper types (avoids one vector lookup). But I
can also change it when I add the wrapper types.

It's too bad we don't have a SmallMap class to give MDInfoTy a good
API and make it efficient at both small and large map sizes.  Maybe
s/MDInfoTy/MDMapTy/ to point out that it's a map from MDKindIds to
MDNodes?

Comment whether RegisterMDKind requires that its argument is
as-yet-unknown in Metadata.h.

Context.pImpl->TheMetadata.ValueIsDeleted(this); in ~Value will always
call the empty ValueIsDeleted(Value*) overload.

Please write a unittest for this.

Metadata::getMDKind(Name) can just be "return MDHandlerNames.lookup(Name)"

Could you comment what the WeakVH in MDPairTy is following? I got
confused and thought it was tracking the Instruction holding the
metadata rather than the MDNode.


You did say this was partial; sorry if I pointed out anything you were
already planning to do.

Jeffrey