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

[David Blaikie]

On Mon, Nov 3, 2014 at 11:22 AM, Duncan P. N. Exon Smith <
dexonsmith at apple.com> wrote:

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

Yeah, as you said, that seems entirely circular/contradiction - you're
going to make most of the metadata into Users/Values anyway.


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

That's also going to be true of the new MDUser you're introducing, right?
It'll have to have two operand counters.


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

Also true of MDUser, no?


>
> 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.
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141103/06946020/attachment.html>