<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 3, 2014 at 11:22 AM, 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"><span class=""><br>
> On 2014-Nov-03, at 11:10, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Mon, Nov 3, 2014 at 11:06 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
> > On 2014-Nov-03, at 10:40, David Blaikie <<a href="mailto:dblaikie@gmail.com">dblaikie@gmail.com</a>> wrote:<br>
> ><br>
> ><br>
> ><br>
> > On Mon, Nov 3, 2014 at 10:13 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
> > Author: dexonsmith<br>
> > Date: Mon Nov  3 12:13:57 2014<br>
> > New Revision: 221167<br>
> ><br>
> > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=221167&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=221167&view=rev</a><br>
> > Log:<br>
> > IR: MDNode => Value: Instruction::getAllMetadataOtherThanDebugLoc()<br>
> ><br>
> > Change `Instruction::getAllMetadataOtherThanDebugLoc()` from a vector of<br>
> > `MDNode` to one of `Value`.  Part of PR21433.<br>
> ><br>
> > This does seem like a rather unfortunate loss in type safety - is there no common type here other than Value*? Should we introduce one.<br>
> ><br>
> > (more broadly, I do wonder if there's some common API we could expose via MDNode so as not to end up with two kinds of metadata... but I've not really been able to wrap my head around this enough to have a well informed perspective)<br>
> ><br>
><br>
> Because of the overlap in API between `User` and `MDNode` -- they both<br>
> have operands, but deal with them completely differently -- the only<br>
> sane common base class between `MDNode` and `MDUser`/`CustomMD` is<br>
> `Value`.  Otherwise, `MDNode` would have to inherit from `User`<br>
> somehow which is (IMO) significantly worse.<br>
><br>
> What's the cost in having MDNode inherit from User (and thus, indirectly, from Value)? You mention that we already pretty regularly have Values that aren't Users - would this instance be particularly worse? Is any instance so bad that it's worth doing these other contortions instead?<br>
><br>
<br>
</span>Two things are pretty bad with it.<br>
<br>
 1. `MDNode` is by far the most numerous `Value`, and this would a<br>
    pointer to `sizeof(MDNode)` for the (never unused)<br>
    `User::OperandList`.  This seems wasteful, although I suppose it's<br>
    only temporary since I'll be getting rid of most of them.<br></blockquote><div><br></div><div>Yeah, as you said, that seems entirely circular/contradiction - you're going to make most of the metadata into Users/Values anyway.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> 2. `MDNode` couldn't reuse `Value::NumOperands`, since that would<br>
    confuse `User`.  It would have it's own `NumOperands`, shadowing the<br>
    `protected` one in a confusing way (on x86-64, this wouldn't<br>
    actually cause an increase in `sizeof(MDNode)` though).<br></blockquote><div><br></div><div>That's also going to be true of the new MDUser you're introducing, right? It'll have to have two operand counters.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">    Morever, it would have a full operand API shadowing `User`'s that is<br>
    completely unrelated.<br></blockquote><div><br>Also true of MDUser, no?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I think both solutions are ugly, but IMO having `MDNode` inherit somehow<br>
from `User` is the greater evil.<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
> I threw some thoughts on a longer term fix into PR21438 [1] last week<br>
> though, since this is bothering me too.<br>
><br>
> [1]: <a href="http://llvm.org/bugs/show_bug.cgi?id=21438" target="_blank">http://llvm.org/bugs/show_bug.cgi?id=21438</a><br>
><br>
> Right, I vaguely recall reading that - thanks for the (re)pointer.<br>
<br>
</div></div></blockquote></div><br></div></div>