Skip to content

[LLVM] Add LLVMIntrinsicCopyName to LLVM C API #112197

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
+74 −16
Draft

[LLVM] Add LLVMIntrinsicCopyName to LLVM C API #112197

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions llvm/docs/ReleaseNotes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,10 @@ Changes to the C API

* Added `LLVMAtomicRMWBinOpUSubCond` and `LLVMAtomicRMWBinOpUSubSat` to `LLVMAtomicRMWBinOp` enum for AtomicRMW instructions.

* Added `LLVMIntrinsicCopyName` and changed `LLVMIntrinsicCopyOverloadedName`
and `LLVMIntrinsicCopyOverloadedName2` to return `char *` instead of
`const char *`.

Changes to the CodeGen infrastructure
-------------------------------------

Expand Down
21 changes: 13 additions & 8 deletions llvm/include/llvm-c/Core.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2833,11 +2833,17 @@ LLVMTypeRef LLVMIntrinsicGetType(LLVMContextRef Ctx, unsigned ID,
*/
const char *LLVMIntrinsicGetName(unsigned ID, size_t *NameLength);

/**
* Copies the name of an intrinsic. The caller is responsible for freeing the
* returned string.
*
* @see llvm::Intrinsic::getName()
*/
char *LLVMIntrinsicCopyName(unsigned ID, size_t *NameLength);

/** Deprecated: Use LLVMIntrinsicCopyOverloadedName2 instead. */
const char *LLVMIntrinsicCopyOverloadedName(unsigned ID,
LLVMTypeRef *ParamTypes,
size_t ParamCount,
size_t *NameLength);
char *LLVMIntrinsicCopyOverloadedName(unsigned ID, LLVMTypeRef *ParamTypes,
Copy link
Contributor

@arsenm arsenm Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think requiring clients to now free this is a pretty bad breaking change. As far as I understand this is the only way for the C API to interact with intrinsics, so the allocation is an additional cost to them. I'm still skeptical of this plan to start allocating intrinsic names

Copy link
Contributor Author

@jurahul jurahul Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an existing API, but just changing the return type to char * instead of const char * to indicate that the ownership of the allocated memory is not LLVM but the client. Maybe I should separate this into its own change (i.e., one to change return type of current API and another to add the new API)

Copy link
Contributor Author

@jurahul jurahul Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For overloaded intrinsics, we always need to construct a new string as the mangling suffix is not in the table

Copy link
Contributor Author

@jurahul jurahul Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still skeptical of this plan to start allocating intrinsic names

Yeah, I guess may be the !/$ is not as much in trying to compress name table by dropping suffixes (given the amount of complexity). Additionally, trimming target specific intrinsics (which I am, unsuccessfully yet, trying to get closure on w.r.t Intrinsic:ID ABI/stability guarantees) is much bigger !/$, so may make sense to focus on that. Let me do that for now, and just sent a fix to change the return types of the 2 existing API to char *.

Copy link
Contributor Author

@jurahul jurahul Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started #114334 for just the existing API fixup

size_t ParamCount, size_t *NameLength);

/**
* Copies the name of an overloaded intrinsic identified by a given list of
Expand All @@ -2850,10 +2856,9 @@ const char *LLVMIntrinsicCopyOverloadedName(unsigned ID,
*
* @see llvm::Intrinsic::getName()
*/
const char *LLVMIntrinsicCopyOverloadedName2(LLVMModuleRef Mod, unsigned ID,
LLVMTypeRef *ParamTypes,
size_t ParamCount,
size_t *NameLength);
char *LLVMIntrinsicCopyOverloadedName2(LLVMModuleRef Mod, unsigned ID,
LLVMTypeRef *ParamTypes,
size_t ParamCount, size_t *NameLength);

/**
* Obtain if the intrinsic identified by the given ID is overloaded.
Expand Down
20 changes: 12 additions & 8 deletions llvm/lib/IR/Core.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2478,28 +2478,32 @@ const char *LLVMIntrinsicGetName(unsigned ID, size_t *NameLength) {
return Str.data();
}

char *LLVMIntrinsicCopyName(unsigned ID, size_t *NameLength) {
Copy link
Contributor Author

@jurahul jurahul Oct 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new API that @nhaehnle proposed on the discourse thread

auto IID = llvm_map_to_intrinsic_id(ID);
auto Str = llvm::Intrinsic::getName(IID);
*NameLength = Str.size();
return strdup(Str.data());
}

LLVMTypeRef LLVMIntrinsicGetType(LLVMContextRef Ctx, unsigned ID,
LLVMTypeRef *ParamTypes, size_t ParamCount) {
auto IID = llvm_map_to_intrinsic_id(ID);
ArrayRef<Type*> Tys(unwrap(ParamTypes), ParamCount);
return wrap(llvm::Intrinsic::getType(*unwrap(Ctx), IID, Tys));
}

const char *LLVMIntrinsicCopyOverloadedName(unsigned ID,
LLVMTypeRef *ParamTypes,
size_t ParamCount,
size_t *NameLength) {
char *LLVMIntrinsicCopyOverloadedName(unsigned ID, LLVMTypeRef *ParamTypes,
size_t ParamCount, size_t *NameLength) {
auto IID = llvm_map_to_intrinsic_id(ID);
ArrayRef<Type*> Tys(unwrap(ParamTypes), ParamCount);
auto Str = llvm::Intrinsic::getNameNoUnnamedTypes(IID, Tys);
*NameLength = Str.length();
return strdup(Str.c_str());
}

const char *LLVMIntrinsicCopyOverloadedName2(LLVMModuleRef Mod, unsigned ID,
LLVMTypeRef *ParamTypes,
size_t ParamCount,
size_t *NameLength) {
char *LLVMIntrinsicCopyOverloadedName2(LLVMModuleRef Mod, unsigned ID,
LLVMTypeRef *ParamTypes,
size_t ParamCount, size_t *NameLength) {
auto IID = llvm_map_to_intrinsic_id(ID);
ArrayRef<Type *> Tys(unwrap(ParamTypes), ParamCount);
auto Str = llvm::Intrinsic::getName(IID, Tys, unwrap(Mod));
Expand Down
45 changes: 45 additions & 0 deletions llvm/unittests/IR/IntrinsicsTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
//===----------------------------------------------------------------------===//

#include "llvm/IR/Intrinsics.h"
#include "llvm-c/Core.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/IR/Constant.h"
#include "llvm/IR/IRBuilder.h"
Expand Down Expand Up @@ -127,6 +128,50 @@ TEST(IntrinsicNameLookup, MSBuiltinLookup) {
EXPECT_EQ(ID, getIntrinsicForMSBuiltin(Target, Builtin));
}

// Test C API to get/copy LLVM intrinsic name.
TEST(IntrinsicNameLookup, LLVMIntrinsicGetCopyNameSimple) {
static constexpr struct {
Intrinsic::ID ID;
const char *Name;
} Tests[] = {{Intrinsic::not_intrinsic, "not_intrinsic"},
{Intrinsic::assume, "llvm.assume"},
{Intrinsic::coro_free, "llvm.coro.free"},
{Intrinsic::aarch64_break, "llvm.aarch64.break"},
{Intrinsic::x86_int, "llvm.x86.int"}};

for (auto [ID, ExpectedName] : Tests) {
size_t NameSize = 0;
const char *CName = LLVMIntrinsicGetName(ID, &NameSize);
StringRef Name(CName, NameSize);

// Verify we get correct name.
EXPECT_EQ(Name, ExpectedName);
const char *CName1 = LLVMIntrinsicGetName(ID, &NameSize);

// Verify we get the same pointer and length the second time.
EXPECT_EQ(CName, CName1);
EXPECT_EQ(NameSize, Name.size());
}

// Now test the copy API.
for (auto [ID, ExpectedName] : Tests) {
size_t NameSize = 0;
char *CName = LLVMIntrinsicCopyName(ID, &NameSize);
StringRef Name(CName, NameSize);

// Verify we get correct name.
EXPECT_EQ(Name, ExpectedName);

// Verify we get the different pointer and same length the second time.
char *CName1 = LLVMIntrinsicCopyName(ID, &NameSize);
EXPECT_NE(CName, CName1);
EXPECT_EQ(NameSize, Name.size());

free(CName);
free(CName1);
}
}

TEST_F(IntrinsicsTest, InstrProfInheritance) {
auto isInstrProfInstBase = [](const Instruction &I) {
return isa<InstrProfInstBase>(I);
Expand Down
Loading