Add an alternative to LLVMMDNode that will only accept metadata as argument.

[deadalnix]

Summary

Once we get a set of function that accept metadata only as argument consitently, and deprecate the alternatives, we'll be able to introduce metadata as its own type.

[jyknight]

I don't understand this change (maybe the description just needs more context?).

Is the intention to later introduce an LLVMMetadataRef type, and these new functions will use that instead of LLVMValueRef, then? If so, why isn't introducing them with LLVMValueRef now problematic?

[deadalnix]

@jyknight Historically, Metadata were some kind of Value. But now, Metadata has its own hierarchy. In order to expose this in the C API, we need a transition plan.

The main problem here is that LLVMMDNode accept both metadata and values. Because of this, it is impossible to split the 2 right now. In order to get out of there, I want to go through a deprecation cycle for LLVMMDNode in favor of an alternative that will only accept metadata.

Right now, these metadata are wrapped as value, but passing down anything else than a metadata result in an error. As a result, it is possible to switch to LLVMMetadataRef the method that produce and consume metadata at once in such a way that thing still works with outdated headers.

[deadalnix]

Restore function that were uintentianally commented out.

[jyknight]

In D19641#414987, @deadalnix wrote:

@jyknight Historically, Metadata were some kind of Value. But now, Metadata has its own hierarchy. In order to expose this in the C API, we need a transition plan.

The main problem here is that LLVMMDNode accept both metadata and values. Because of this, it is impossible to split the 2 right now. In order to get out of there, I want to go through a deprecation cycle for LLVMMDNode in favor of an alternative that will only accept metadata.

Right now, these metadata are wrapped as value, but passing down anything else than a metadata result in an error. As a result, it is possible to switch to LLVMMetadataRef the method that produce and consume metadata at once in such a way that thing still works with outdated headers.

Okay, so you're saying that in the future, all of the things that currently say "LLVMValueRef" but are talking about metadata will be changed to say "LLVMMetadataRef", but that it will still work transparently even with old software at that point, because LLVMValueAsMetadata/LLVMGetMDNode will also be changed to return LLVMMetadataRef at the same time.

Changing the types of the functions seems somewhat questionable, but I guess it would work -- as long as people don't fail to call, (or call extraneously/incorrectly!) the new LLVMMetadataAsValue and LLVMValueAsMetadata functions.

But, those functions are really confusing right now, because they're basically no-ops (takes a value and returns the same value!), so I'd kinda expect basically everyone to get their usage wrong in the intervening period.

How many functions would need to be added if you just added LLVMMetadataRef now, along with a new version of every function that operates on metadata disguised as a value, which would operate directly on the metadata instead?

[deadalnix]

Okay, so you're saying that in the future, all of the things that currently say "LLVMValueRef" but are talking about metadata will be changed to say "LLVMMetadataRef", but that it will still work transparently even with old software at that point, because LLVMValueAsMetadata/LLVMGetMDNode will also be changed to return LLVMMetadataRef at the same time.

Yes.

Changing the types of the functions seems somewhat questionable, but I guess it would work -- as long as people don't fail to call, (or call extraneously/incorrectly!) the new LLVMMetadataAsValue and LLVMValueAsMetadata functions.

These are opaque pointers, so that alright ABIwise. Going through the deprecation of LLVMMDNode will force people to do the right thing or get a runtime error.

But, those functions are really confusing right now, because they're basically no-ops (takes a value and returns the same value!), so I'd kinda expect basically everyone to get their usage wrong in the intervening period.

They aren't noop except LLVMMetadataAsValue, which simply checks that you pass it a metatdata. This one is not strictly necessary at this stage so maybe I can just remove it.

How many functions would need to be added if you just added LLVMMetadataRef now, along with a new version of every function that operates on metadata disguised as a value, which would operate directly on the metadata instead?

13 if I'm not mistaken (including LLVMMDNode, which needs duplication either way).

[Wallbraker]

I strongly feel that we should just add new function and then remove the old ones the release after.

You are changing API, we do that in LLVM by adding new functions, then removing the old ones. The fact that the type sizes line up is just a happy coincidence. Doing it your way adds more risks its not certain that you have covered all of the use cases, adding duplicated functions makes sure we get all of them because we are actually using the type system to work for us. And adding new functions also lets us get a clear view of what the new API will look like.

And its not like we are going to have 13 more functions forever, its just for a release.

[deadalnix]

It means we are breaking everybody's code for no good reason.

[deadalnix]

Remove noop function LLVMMetadataAsValue.

[deadalnix]

Use LLVMCreateMDNode as it is more consistent with existing names.

[whitequark]

Do I understand it correctly that LLVMMetadataRef is going to effectively become a subtype of LLVMValueRef? I.e. it is (and will be, indefinitely) legal to pass an LLVMMetadataRef anywhere you can pass LLVMValueRef, but not vice versa?

Further, do I understand it correctly that the final signatures should look something like:

LLVMMetadataRef LLVMMDString(const char *Str, unsigned SLen)
LLVMMetadataRef LLVMCreateMDNode(LLVMContextRef C, LLVMMetadataRef *Vals, unsigned Count)
LLVMMetadataRef LLVMValueAsMetadata(LLVMValueRef Val)

and so on?

And is it correct that LLVMMDNode function should be deprecated because everywhere else, the taken LLVMValueRef already *must* refer to metadata, but that's not true for LLVMMDNode?

In that case I agree with @deadalnix that we should not deprecate these 13 functions as this is not really a breakage of ABI contract, and it is sufficient to go through a single deprecation cycle removing just the LLVMMDNode function.

[whitequark]

I think it's fine to land this but only if we introduce LLVMMetadataRef at the same time, otherwise it becomes nearly impossible to understand for downstream.

docs/ReleaseNotes.rst

 * The C API function LLVMAddTargetData has been removed.
 
 * The C API function LLVMGetDataLayout is deprecated
   in favor of LLVMGetDataLayoutStr.
 
 * The C API enum LLVMAttribute is deprecated in favor of
   LLVMGetAttrKindID.
 
+* The C API function LLVMMDNodeis deprecated in favor of LLVMGetMDNode.
+
 * ``TargetFrameLowering::eliminateCallFramePseudoInstr`` now returns an
   iterator to the next instruction instead of ``void``. Targets that previously
   did ``MBB.erase(I); return;`` now probably want ``return MBB.erase(I);``.
 
 .. NOTE
    For small 1-3 sentence descriptions, just add an entry at the end of
    this list. If your description won't fit comfortably in one bullet
    point (e.g. maybe you would like to give an example of the

include/llvm-c/Core.h

  */
 LLVMValueRef LLVMMDString(const char *Str, unsigned SLen);
 
 /**
  * Obtain a MDNode value from a context.
  *
  * The returned value corresponds to the llvm::MDNode class.
  */
-LLVMValueRef LLVMMDNodeInContext(LLVMContextRef C, LLVMValueRef *Vals,
-                                 unsigned Count);
+// LLVMValueRef LLVMMDNodeInContext(LLVMContextRef C, LLVMValueRef *Vals,
+//                                  unsigned Count);
 
 /**
  * Obtain a MDNode value from the global context.
  */
-LLVMValueRef LLVMMDNode(LLVMValueRef *Vals, unsigned Count);
+// LLVMValueRef LLVMMDNode(LLVMValueRef *Vals, unsigned Count);
+
+/**
+ * Obtain a MDNode metadata from the global context.
+ *
+ * While LLVMMDNode and LLVMMDNodeInContext will accept a variety
+ * of inputs, LLVMGetMDNode will only accept metadata as arguments.
+ */
+LLVMValueRef LLVMGetMDNode(LLVMContextRef C, LLVMValueRef *Vals,
+                           unsigned Count);
+
+/**
+ * Obtain a Metadata as a Value.
+ */
+LLVMValueRef LLVMMetadataAsValue(LLVMContextRef C, LLVMValueRef MD);
+
+/**
+ * Obtain a Value as a Metadata.
+ */
+LLVMValueRef LLVMValueAsMetadata(LLVMValueRef Val);
 
 /**
  * Obtain the underlying string from a MDString value.
  *
  * @param V Instance to obtain string from.
  * @param Length Memory address which will hold length of returned string.
  * @return String data in MDString.
  */

lib/IR/Core.cpp

LLVMValueRef LLVMMDNodeInContext(LLVMContextRef C, LLVMValueRef *Vals,
   }
   return wrap(MetadataAsValue::get(Context, MDNode::get(Context, MDs)));
 }
 
 LLVMValueRef LLVMMDNode(LLVMValueRef *Vals, unsigned Count) {
   return LLVMMDNodeInContext(LLVMGetGlobalContext(), Vals, Count);
 }
 
+LLVMValueRef LLVMGetMDNode(LLVMContextRef C, LLVMValueRef *Vals,
+                           unsigned Count) {
+  LLVMContext &Context = *unwrap(C);
+  SmallVector<Metadata *, 8> MDs;
+  for (auto *V : makeArrayRef(Vals, Count))
+    MDs.push_back(unwrap<MetadataAsValue>(V)->getMetadata());
+  return wrap(MetadataAsValue::get(Context, MDNode::get(Context, MDs)));
+}
+
+LLVMValueRef LLVMMetadataAsValue(LLVMContextRef C, LLVMValueRef MD) {
+  return wrap(unwrap<MetadataAsValue>(MD));
+}
+
+LLVMValueRef LLVMValueAsMetadata(LLVMValueRef Val) {
+  auto *V = unwrap(Val);
+  Metadata *MD;
+  if (auto *C = dyn_cast<Constant>(V))
+    MD = ConstantAsMetadata::get(C);
+  if (auto *MAV = dyn_cast<MetadataAsValue>(V))
+    MD = MAV->getMetadata();
+  else
+    MD = ValueAsMetadata::get(V);
+  return wrap(MetadataAsValue::get(V->getContext(), MD));
+}
+
 const char *LLVMGetMDString(LLVMValueRef V, unsigned *Length) {
   if (const auto *MD = dyn_cast<MetadataAsValue>(unwrap(V)))
     if (const MDString *S = dyn_cast<MDString>(MD->getMetadata())) {
       *Length = S->getString().size();
       return S->getString().data();
     }
   *Length = 0;
   return nullptr;

tools/llvm-c-test/metadata.c

 |* This file implements the --add-named-metadata-operand and --set-metadata   *|
 |* commands in llvm-c-test.                                                   *|
 |*                                                                            *|
 \*===----------------------------------------------------------------------===*/
 
 #include "llvm-c-test.h"
 
 int llvm_add_named_metadata_operand(void) {
-  LLVMModuleRef m = LLVMModuleCreateWithName("Mod");
-  LLVMValueRef values[] = { LLVMConstInt(LLVMInt32Type(), 0, 0) };
+  LLVMContextRef C = LLVMGetGlobalContext();
+  LLVMModuleRef M = LLVMModuleCreateWithName("Mod");
+  LLVMValueRef Values[] = {
+    LLVMValueAsMetadata(LLVMConstInt(LLVMInt32Type(), 0, 0))
+  };
 
   // This used to trigger an assertion
-  LLVMAddNamedMetadataOperand(m, "name", LLVMMDNode(values, 1));
+  LLVMAddNamedMetadataOperand(M, "name", LLVMGetMDNode(C, Values, 1));
 
-  LLVMDisposeModule(m);
+  LLVMDisposeModule(M);
 
   return 0;
 }
 
 int llvm_set_metadata(void) {
-  LLVMBuilderRef b = LLVMCreateBuilder();
-  LLVMValueRef values[] = { LLVMConstInt(LLVMInt32Type(), 0, 0) };
+  LLVMBuilderRef B = LLVMCreateBuilder();
+  LLVMValueRef Values[] = {
+    LLVMValueAsMetadata(LLVMConstInt(LLVMInt32Type(), 0, 0))
+  };
 
   // This used to trigger an assertion
   LLVMSetMetadata(
-      LLVMBuildRetVoid(b),
+      LLVMBuildRetVoid(B),
       LLVMGetMDKindID("kind", 4),
-      LLVMMDNode(values, 1));
+      LLVMGetMDNode(LLVMGetGlobalContext(), Values, 1));
 
-  LLVMDisposeBuilder(b);
+  LLVMDisposeBuilder(B);
 
   return 0;
 }