[LLVMdev] [RFC] Separating Metadata from the Value hierarchy

[Chris Lattner]

> On Nov 9, 2014, at 5:02 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> In response to my recent commits (e.g., [1]) that changed API from
> `MDNode` to `Value`, Eric had a really interesting idea [2] -- split
> metadata entirely from the `Value` hierarchy, and drop general support
> for metadata RAUW.

Wow, this never occurred to me, but in hindsight seems like obviously the right direction.

> Here is what we'd lose:
> 
> 1. No more arbitrary RAUW of metadata.
> 
>    While we'd keep support for RAUW of temporary MDNodes for use as
>    forward declarations (when parsing assembly or constructing cyclic
>    graphs), drop RAUW support for all other metadata.

This seems fine to me, a corollary of this should be that MDNodes never “move around” in memory due to late uniquing etc, which means that TrackingVH shouldn’t be necessary, right?  This should make all frontends a lot more memory efficient because they can just use raw pointers to MDNodes everywhere.

> 2. No more function-local metadata.

This was a great idea that never mattered, I’m happy to drop it for the greater good :-)

Some comments on the patch-in-progress:

+++ b/include/llvm/IR/MetadataV2.h
+#include "llvm/ADT/ArrayRef.h"
+#include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/PointerUnion.h"
+#include "llvm/ADT/StringMap.h"
+#include "llvm/IR/Value.h"
+#include "llvm/Support/ErrorHandling.h"

Please factor things to avoid including heavy headers like DenseMap and StringMap (e.g. by moving the Impl stuff out).

+class Metadata {
+private:
+  LLVMContext &Context;

Out of curiosity, why do Metadata nodes all need to carry around a context?  This seems like memory bloat that would be great to avoid if possible.



+template <class T> class TypedMDRef : public MDRef {

Is this a safe way to model this, should it use private inheritance instead?  Covariance seems like it would break this: specifically something could pass off a typed MDRef as an MDRef&, then the client could reassign something of the wrong type into it.


+/// \note This is really cheap and easy to support, and it's useful for
+/// implementing:

When this makes more progress, the comments should explain what it does and how it works, out of context of the patch.


+  MDString(LLVMContext *Context) : Metadata(*Context, MDStringKind) {
+    assert(Context && "Expected non-null context");
...
+  static MDStringRef get(LLVMContext &Context, StringRef String);

You have reference/pointer disagreement, I’d recommend taking LLVMContext& to the ctor for uniformity (and remove the assert).

+class ReplaceableMetadataImpl {

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.

+class ValueAsMetadata : public Metadata, ReplaceableMetadataImpl {

I think that ValueAsMetadata is a great thing to have - it is essential to refer to global variables etc.  That said, I think that it will eventually be worthwhile to have metadata versions of integers and other common things to reduce memory.  This should come in a later pass of course.

I don’t follow the point of
+class UniquableMDNode : public MDNode {

What would subclass it?  What is the use-case?  Won’t MDStrings continue to be uniqued?

Overall, I’m thrilled to see this direction, thanks for pushing forward on it Duncan!

-Chris