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

[David Blaikie]

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

>
> > On 2014-Nov-03, at 11:30, David Blaikie <dblaikie at gmail.com> wrote:
> >
> >
> >
> > 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?
>
> No, `MDUser` uses the API directly from `User`.
>
> Some subclasses will add a `ValueHandle`, but it'll be treated differently
> and it won't shadow the `User` API.
>

Ah, rightio. :s


>
> >
> >
> > 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/325d71f9/attachment.html>