<div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
> I’m not following your algorithm intentions well yet, but this seems like a really heavy-weight implementation, given that this is a common base class for a number of other important things.<br>
<br>
Unfortunately I think this weight will be hard to optimize out (although<br>
I'm sure it could be a little smaller).<br>
<br>
In the `Value` hierarchy, you pay memory for RAUW at the site of each<br>
`Use`.  This makes sense, since most values can be RAUW'ed.<br>
<br>
Since very little metadata needs to be RAUW'ed, we don't want to pay<br>
memory at every use-site.  Instead, we pay the cost inside the<br>
(hopefully) few instances that support RAUW.<br></blockquote><div><br></div><div>Agreed. I don't see us needing to RAUW debug info outside of llvm-link and the forward references. For other metadata uses RAUW doesn't seem to be used at all (at least a quick glance didn't show TempMDNodes anywhere).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The core assumption is that there are relatively few replaceable<br>
metadata instances.  The only subclasses are `ValueAsMetadata` and<br>
`TempMDNode`.  How many of these are live?<br>
<br>
  - There will be at most one `ValueAsMetadata` instance for each<br>
    metadata operand that points at a `Value`.  My data from a couple of<br>
    months ago showed that there are very few of these.<br>
<br>
  - `TempMDNodes` are used as forward references.  You only pay their<br>
    cost until the forward reference gets resolved (i.e., deleted).<br>
<br>
The main case I'm worried about here are references to `ConstantInt`s,<br>
which are used pretty heavily outside of debug info (e.g., in `!prof`<br>
attachments).  However, if that shows up in a profile, we can bypass<br>
`Value`s entirely by creating a new `MDIntArray` subclass (or<br>
something).<br>
<br></blockquote><div><br></div><div>This makes sense.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
The class hierarchy I envision looks something like this:<br>
<br>
    Metadata<br>
      MDNode<br>
        TempMDNode      // MDNodeFwdRef?<br>
        UniquableMDNode // GenericMDNode?<br>
        DINode          // Is this layer useful?<br>
          DILocation<br>
          DIScope<br>
            DIType<br>
              DIBasicType<br>
              DICompositeType<br>
              ...<br>
            DISubprogram<br>
            ...<br>
          DICompileUnit<br></blockquote><div><br></div><div>FWIW technically a compile unit is a scope so would probably need to be under that. Otherwise I'm missing a change rationale (I haven't read the full patch yet, I apologize if it's clear from the patch).</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
          ...<br>
      MDString<br>
      ValueAsMetadata<br>
<br>
`UniquableMDNode` is a leaf-class that behaves like the current `MDNode`<br>
(when it's not a temporary).  I called it "uniquable" because, unlike<br>
`TempMDNode`, it is uniqued by default (although you can opt-out of it,<br>
and uniquing might be dropped).<br>
<br>
Maybe a better name is `GenericMDNode`?<br>
<br>
Also, off-topic, but after sketching out the imagined hierarchy above,<br>
I'm not sure `DINode` is particularly useful.<br></blockquote><div><br></div><div>Possibly? Might be from the "organizing all of the metadata types" level?</div><div><br></div><div>-eric </div></div>