Page MenuHomePhabricator

[LLVM-C] Add Bindings For Named Metadata
ClosedPublic

Authored by CodaFi on May 21 2018, 10:36 PM.

Details

Summary

Add a new type for named metadata nodes. Use this to implement iterators and accessors for NamedMDNodes and extend the echo test to use them to copy module-level debug information.

Diff Detail

Repository
rL LLVM

Event Timeline

CodaFi created this revision.May 21 2018, 10:36 PM

@aprantl Let me know if this is the right way to address that FIXME. There didn't seem to be any consequences of the small change I've got here which could be very good or very bad from my naive perspective.

C API changes LGTM.

Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.

Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.

Making NamedMDNode inherit from Metadata will allow MDNode to reference NamedMDNode.

  • What's the use case for this?
  • What complexity does it add to the IR? (Especially, during serialization and LTO...)

Perhaps this is worth doing, but we need a compelling reason. In retrospect, I'm not sure I had sufficient motiviation when I added the TODO.

Adding Duncan to the review list who (presumably) added the TODO and may have some thoughts on this.

Making NamedMDNode inherit from Metadata will allow MDNode to reference NamedMDNode.

  • What's the use case for this?
  • What complexity does it add to the IR? (Especially, during serialization and LTO...)

Perhaps this is worth doing, but we need a compelling reason. In retrospect, I'm not sure I had sufficient motiviation when I added the TODO.

If the goal is just to expose them to the C API, you can expose them as is by adding LLVMNamedMetadataRef, etc.

CodaFi edited the summary of this revision. (Show Details)Aug 25 2018, 4:25 PM
CodaFi updated this revision to Diff 162569.Aug 25 2018, 4:33 PM

Spoke with @aprantl in person about this, and we came to the conclusion that named metadata nodes are different enough that the original TODO doesn't make any sense. For one, it would mean that you could use named metadata nodes to create more kinds of cycles. It would also mean the verifier would have to be extended to reject any usage of named metadata nodes as operand metadata.

Suffice to say, it's both simpler and more correct to make a new type in the C API and wrap to that.

Could you perhaps replace the TODO in the code with a summary of what you just said?

The C-API changes looks good to me and the logic makes sense. Just one nit.

include/llvm-c/Core.h
898 ↗(On Diff #162569)

Should this be disposed with LLVMDisposeMessage? Please update comment to reflect this.

CodaFi added inline comments.Aug 27 2018, 10:21 AM
include/llvm-c/Core.h
898 ↗(On Diff #162569)

No. In general, Get-named things return objects managed by somebody else. Copy-named things (or, unfortunately, functions documented to the contrary) return user-owned objects. In this case, the module owns the named metadata which owns its name.

Wallbraker added inline comments.Aug 27 2018, 10:36 AM
include/llvm-c/Core.h
898 ↗(On Diff #162569)

Ah okay, so it's not consistent over the API? Even more reason to put it in the doc-comment of this function.

CodaFi added inline comments.Aug 27 2018, 11:36 AM
include/llvm-c/Core.h
898 ↗(On Diff #162569)

The exceptional cases are what deserve doc comments, and they are documented. Otherwise I would have to re-annotate the entire C API which is out of scope for this patch and would be noise.

It would be good to go through and audit these things, though. The C API is quite old and could use some standardization here.

CodaFi updated this revision to Diff 162721.Aug 27 2018, 11:44 AM

@whitequark Could I get another once-over on the C API here?

This revision is now accepted and ready to land.Aug 30 2018, 9:49 AM
This revision was automatically updated to reflect the committed changes.