<div dir="ltr">FWIW, this completely addresses my only ill feelings about the changes you were making. I really like it.<div><br></div><div>I might bikeshed some of the names, but whatever. I would suggest maybe augmenting some of the doxygen comments to help show the specific use case that motivates the node? You already have this in a few places and it really helped me skimming the code. In particular, there are a bunch of very similar nodes around the tracking, temp, uniquable, etc. and I think it'll be important to clearly distinguish each use case that these are designed to address.</div><div><br></div><div>Also just want to say thanks for diving into the design and finding something (at least, I'm hoping!) works even better.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Sun, Nov 9, 2014 at 7:02 PM, Duncan P. N. Exon Smith <span dir="ltr"><<a href="mailto:dexonsmith@apple.com" target="_blank">dexonsmith@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">TL;DR: If you use metadata (especially if it's out-of-tree), check the<br>
numbered list of lost functionality below to see whether I'm trying to<br>
break your compiler permanently.<br>
<br>
In response to my recent commits (e.g., [1]) that changed API from<br>
`MDNode` to `Value`, Eric had a really interesting idea [2] -- split<br>
metadata entirely from the `Value` hierarchy, and drop general support<br>
for metadata RAUW.<br>
<br>
After hacking together some example code, this seems overwhelmingly to<br>
me like the right direction. See the attached metadata-v2.patch for my<br>
sketch of what the current metadata primitives might look like in the<br>
new hierarchy (includes LLVMContextImpl uniquing support).<br>
<br>
The initial motivation was to avoid the API weaknesses caused by having<br>
non-`MDNode` metadata that inherits from `Value`. In particular,<br>
instead of changing API from `MDNode` to `Value`, change it to a base<br>
class called `Metadata`, which sheds the underused and expensive `Value`<br>
base class entirely.<br>
<br>
The implications are broader: an enormous reduction in complexity (and<br>
overhead) for metadata.<br>
<br>
This change implies minor (major?) loss of functionality for metadata,<br>
but Eric and I suspect that the only hard-to-fix user is debug info<br>
itself, whose IR infrastructure I'm rewriting anyway.<br>
<br>
Here is what we'd lose:<br>
<br>
1. No more arbitrary RAUW of metadata.<br>
<br>
While we'd keep support for RAUW of temporary MDNodes for use as<br>
forward declarations (when parsing assembly or constructing cyclic<br>
graphs), drop RAUW support for all other metadata.<br>
<br>
Note that we'd also keep support for RAUW of `Value` operands of<br>
metadata.<br>
<br>
If the RAUW of an operand causes a uniquing collision, uniquing<br>
would be dropped for that node. This matches the current behaviour<br>
when an operand goes to null.<br>
<br>
Upgrade path: none.<br>
<br>
2. No more function-local metadata.<br>
<br>
AFAICT, function-local metadata is *only* used for indirect<br>
references to instructions and arguments in `@llvm.dbg.value` and<br>
`@llvm.dbg.declare` intrinsics. The first argument of the following<br>
is an example:<br>
<br>
call void @llvm.dbg.value(metadata !{i32 %val}, metadata !0,<br>
metadata !1)<br>
<br>
Note that the debug info people uniformly seem to dislike the status<br>
quo, since it's awkward to get from a `Value` to the corresponding<br>
intrinsic.<br>
<br>
Upgrade path: Instead of using an intrinsic that references a<br>
function-local value through an `MDNode`, attach metadata to the<br>
corresponding argument or instruction, or to the terminating<br>
instruction of the basic block. (This requires new support for<br>
attaching metadata to function arguments, which I'll have to add for<br>
debug info anyway.)<br>
<br>
Is this going to break your compiler? How? Why is your use case worth<br>
supporting?<br>
<br>
[1]: <a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141103/242667.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20141103/242667.html</a><br>
"r221167 - IR: MDNode => Value: Instruction::getAllMetadataOtherThanDebugLoc()"<br>
[2]: <a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078581.html" target="_blank">http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-November/078581.html</a><br>
"Re: First-class debug info IR: MDLocation"<br>
<br>
<br>_______________________________________________<br>
LLVM Developers mailing list<br>
<a href="mailto:LLVMdev@cs.uiuc.edu">LLVMdev@cs.uiuc.edu</a> <a href="http://llvm.cs.uiuc.edu" target="_blank">http://llvm.cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev</a><br>
<br></blockquote></div><br></div>