[llvm] r221167 - IR: MDNode => Value: Instruction::getAllMetadataOtherThanDebugLoc()

[Duncan P. N. Exon Smith]

> On 2014-Nov-03, at 11:10, David Blaikie <dblaikie at gmail.com> wrote:
> 
> 
> 
> On Mon, Nov 3, 2014 at 11:06 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> > On 2014-Nov-03, at 10:40, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > On Mon, Nov 3, 2014 at 10:13 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> > Author: dexonsmith
> > Date: Mon Nov  3 12:13:57 2014
> > New Revision: 221167
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=221167&view=rev
> > Log:
> > IR: MDNode => Value: Instruction::getAllMetadataOtherThanDebugLoc()
> >
> > Change `Instruction::getAllMetadataOtherThanDebugLoc()` from a vector of
> > `MDNode` to one of `Value`.  Part of PR21433.
> >
> > This does seem like a rather unfortunate loss in type safety - is there no common type here other than Value*? Should we introduce one.
> >
> > (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)
> >
> 
> Because of the overlap in API between `User` and `MDNode` -- they both
> have operands, but deal with them completely differently -- the only
> sane common base class between `MDNode` and `MDUser`/`CustomMD` is
> `Value`.  Otherwise, `MDNode` would have to inherit from `User`
> somehow which is (IMO) significantly worse.
> 
> 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?
>  

Two things are pretty bad with it.

 1. `MDNode` is by far the most numerous `Value`, and this would a
    pointer to `sizeof(MDNode)` for the (never unused)
    `User::OperandList`.  This seems wasteful, although I suppose it's
    only temporary since I'll be getting rid of most of them.

 2. `MDNode` couldn't reuse `Value::NumOperands`, since that would
    confuse `User`.  It would have it's own `NumOperands`, shadowing the
    `protected` one in a confusing way (on x86-64, this wouldn't
    actually cause an increase in `sizeof(MDNode)` though).

    Morever, it would have a full operand API shadowing `User`'s that is
    completely unrelated.

I think both solutions are ugly, but IMO having `MDNode` inherit somehow
from `User` is the greater evil.

> 
> I threw some thoughts on a longer term fix into PR21438 [1] last week
> though, since this is bothering me too.
> 
> [1]: http://llvm.org/bugs/show_bug.cgi?id=21438
> 
> Right, I vaguely recall reading that - thanks for the (re)pointer.