Implement LLVMValueRef independent MDNode operations #1001
Conversation
|
Looks fine! Let's keep this PR opened until you're pretty sure you don't have any additional changes though. |
|
It looks like LLVM 11.1.0 was released: https://github.com/llvm/llvm-project/releases Would you mind testing this out and updating this PR with that new version? |
|
LLVM 12.0.0 is set to release on 2. March, would we want to wait for that or do we also want a build for 11.1? I know 12.0.0 has a few backwards incompatible changes as they have removed OrcJIT v1 from the C API so maybe a 11.1.0 build would be nice for users who do not wish to upgrade yet? |
|
Maybe? Let's see when 12.0.0 comes out...
|
|
Take that back, it seems like the OrcJIT v1 deletion (llvm/llvm-project@6154c41) dates back before 11.0.1 which would mean the current version we are building wouldn't have the Orc bindings anyways. Since LLVM 12.0.0 is in release-candidate state there shouldn't be any more changes to the API before release which means there will be no differences in the C API between 11.1.0 and 12.0.0 unless an error is found during the release cycle. I'll upgrade the preset to 11.1.0 and then we'll see what happens once 12.0.0 rolls out. |
I would once again, like to expand the C API because the original design uses additional conversions for no reason. In this case, wrapping LLVMMetadataRefs into LLVMValues through MetadataAsValue nodes
junlarsen
force-pushed
the
llvm-metadata-operations
branch
from
5aff1e6 to
89997af
Compare
|
Sorry for going back and forth, but it seems the commit I referenced earlier was made onto the 12.x.x branch which meant it wasn't included in 11.1.0. Let's take care of the rest of that stuff once 12.x.x releases. |
I would once again, like to expand the C API because the original design uses additional conversions for no reason. In this case, wrapping LLVMMetadataRefs into LLVMValues through MetadataAsValue nodes.
The reason the LLVM C API is silly by design is because it requires the user to pass a Context to use the three methods. The C API method takes an
LLVMValueRefwhich has to be allvm::MetadataAsValueunder the hood, a type which requires a LLVM context to create. Passing the context and wrapping an object for no reason when the values can be retrieved with the original LLVMMetadataRef.The context is still needed for LLVMGetMDNodeOperands because this does the conversion between ConstantAsMetadata and LLVMValueRef which requires the context. I think it's way better design to explicitly pass the context you wish the converted ConstantAsMetadata instances in, than magically pulling it from a MetadataAsValue.