[LLVMdev] [proposal] Extensible IR metadata

[Devang Patel]

On Wed, Sep 16, 2009 at 9:53 AM, Jeffrey Yasskin <jyasskin at google.com> wrote:
> 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.

that means making llvm::Metadata a friend of llvm::Value. fine.

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

OK

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

ok


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

ok

> 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)"

ok

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

Instruction can hold either MDNode or MDString. MDNode can be replaced
transparently by the MDNode uniquing scheme, so its holder should
always use WeakVH.


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

I meant, this only covers basic metadata storage. Change required in
llvm parser/printer, bitcode reader/writer, IR builder and test cases
are missing :)

-
Devang