[LLVM-C]Add LLVMAddMetadataToInst, deprecated LLVMSetInstDebugLocation.

[fhahn]

Summary

IRBuilder has been updated to support preserving metdata in a more
general manner. This patch adds LLVMAddMetadataToInst and
deprecates LLVMSetInstDebugLocation in favor of the more
general function.

[dexonsmith]

Seems like a nice cleanup, but I think it'd be better for a current debug info maintainer to take a look.

[fhahn]

Ping :) I just realized that I still had this follow-up to 29077ae860bc lying around. Rebased.

[tstellar]

This is an API break to the C API, which is allowed, but is there any way we can keep the old function?

[fhahn]

In D93454#3001992, @tstellar wrote:

This is an API break to the C API, which is allowed, but is there any way we can keep the old function?

We could keep the old function and just have it call AddMetadataToInst. There should be no functional change *if* the user does not use the new functionality to collect non-debug metadata in the builder. Not sure if that's a great option. We could also extend AddMetadataToInst to accept a filter to only copy specific metadata kinds and have LLVMAddMetadataToInst use this filter. But this would also mean that we need to maintain a change IRBuilder which is only there for C API compatibility. In that case, we might as well keep the old SetInstDebugLocation. What do you think?

[tstellar]

In D93454#3005834, @fhahn wrote:

In D93454#3001992, @tstellar wrote:

This is an API break to the C API, which is allowed, but is there any way we can keep the old function?

We could keep the old function and just have it call AddMetadataToInst. There should be no functional change *if* the user does not use the new functionality to collect non-debug metadata in the builder. Not sure if that's a great option. We could also extend AddMetadataToInst to accept a filter to only copy specific metadata kinds and have LLVMAddMetadataToInst use this filter. But this would also mean that we need to maintain a change IRBuilder which is only there for C API compatibility. In that case, we might as well keep the old SetInstDebugLocation. What do you think?

To me it sounds like keeping the old function would be best. We can also deprecate it in the release notes so that users are aware that it might be removed at some point, and that it might not work with the new functionality.

[fhahn]

In D93454#3006357, @tstellar wrote:

In D93454#3005834, @fhahn wrote:

In D93454#3001992, @tstellar wrote:

This is an API break to the C API, which is allowed, but is there any way we can keep the old function?

We could keep the old function and just have it call AddMetadataToInst. There should be no functional change *if* the user does not use the new functionality to collect non-debug metadata in the builder. Not sure if that's a great option. We could also extend AddMetadataToInst to accept a filter to only copy specific metadata kinds and have LLVMAddMetadataToInst use this filter. But this would also mean that we need to maintain a change IRBuilder which is only there for C API compatibility. In that case, we might as well keep the old SetInstDebugLocation. What do you think?

To me it sounds like keeping the old function would be best. We can also deprecate it in the release notes so that users are aware that it might be removed at some point, and that it might not work with the new functionality.

Sounds good, I updated the patch to only add LLVMAddMetadataToInst and deprecate LLVMAddMetadataToInst.

[aprantl]

Does Doxygen have a \deprecated keyword? Otherwise this LGTM.

[fhahn]

In D93454#3021309, @aprantl wrote:

Does Doxygen have a \deprecated keyword? Otherwise this LGTM.

Yes, I added @deprecated to the paragraph in the comment.

Thanks!

llvm/docs/ReleaseNotes.rst

 
 Changes to the OCaml bindings
 -----------------------------
 
 
 Changes to the C API
 --------------------
 
-* ...
+* ``LLVMSetInstDebugLocation`` has been deprecated in favor of the more general
+  ``LLVMAddMetadataToInst``.
 
 Changes to the Go bindings
 --------------------------
 
 
 Changes to the FastISel infrastructure
 --------------------------------------
 

llvm/include/llvm-c/Core.h

  */
 void LLVMSetCurrentDebugLocation2(LLVMBuilderRef Builder, LLVMMetadataRef Loc);
 
 /**
  * Attempts to set the debug location for the given instruction using the
  * current debug location for the given builder.  If the builder has no current
  * debug location, this function is a no-op.
  *
+ * @deprecated LLVMSetInstDebugLocation is deprecated in favor of the more general
+ *             LLVMAddMetadataToInst.
+ *
  * @see llvm::IRBuilder::SetInstDebugLocation()
  */
 void LLVMSetInstDebugLocation(LLVMBuilderRef Builder, LLVMValueRef Inst);
 
 /**
+ * Adds the metadata registered with the given builder to the given instruction.
+ *
+ * @see llvm::IRBuilder::AddMetadataToInst()
+ */
+void LLVMAddMetadataToInst(LLVMBuilderRef Builder, LLVMValueRef Inst);
+
+/**
  * Get the dafult floating-point math metadata for a given builder.
  *
  * @see llvm::IRBuilder::getDefaultFPMathTag()
  */
 LLVMMetadataRef LLVMBuilderGetDefaultFPMathTag(LLVMBuilderRef Builder);
 
 /**
  * Set the default floating-point math metadata for the given builder.

llvm/lib/IR/Core.cpp

LLVMValueRef LLVMGetCurrentDebugLocation(LLVMBuilderRef Builder) {
   return wrap(MetadataAsValue::get(
       Context, unwrap(Builder)->getCurrentDebugLocation().getAsMDNode()));
 }
 
 void LLVMSetInstDebugLocation(LLVMBuilderRef Builder, LLVMValueRef Inst) {
   unwrap(Builder)->SetInstDebugLocation(unwrap<Instruction>(Inst));
 }
 
+void LLVMAddMetadataToInst(LLVMBuilderRef Builder, LLVMValueRef Inst) {
+  unwrap(Builder)->AddMetadataToInst(unwrap<Instruction>(Inst));
+}
+
 void LLVMBuilderSetDefaultFPMathTag(LLVMBuilderRef Builder,
                                     LLVMMetadataRef FPMathTag) {
 
   unwrap(Builder)->setDefaultFPMathTag(FPMathTag
                                        ? unwrap<MDNode>(FPMathTag)
                                        : nullptr);
 }
 

llvm/tools/llvm-c-test/echo.cpp

auto *AllMetadata =
         LLVMInstructionGetAllMetadataOtherThanDebugLoc(Src,
                                                        &NumMetadataEntries);
     for (unsigned i = 0; i < NumMetadataEntries; ++i) {
       unsigned Kind = LLVMValueMetadataEntriesGetKind(AllMetadata, i);
       LLVMMetadataRef MD = LLVMValueMetadataEntriesGetMetadata(AllMetadata, i);
       LLVMSetMetadata(Dst, Kind, LLVMMetadataAsValue(Ctx, MD));
     }
     LLVMDisposeValueMetadataEntries(AllMetadata);
-    LLVMSetInstDebugLocation(Builder, Dst);
+    LLVMAddMetadataToInst(Builder, Dst);
 
     check_value_kind(Dst, LLVMInstructionValueKind);
     return VMap[Src] = Dst;
   }
 
   LLVMBasicBlockRef DeclareBB(LLVMBasicBlockRef Src) {
     // Check if this is something we already computed.
     {