Map Attribute in the C API.

[deadalnix]

Summary

This is done via function to create attributes, then various setters/getters to set/get attributes on functions/parameters/calls. This only includes the function's return attributes in order to keep things simple while the design is discussed. Once somethign is agreed upon, other accessors can be created.

This API will live together with the enum's one for a while, and the enum's will eventually be removed.

[Wallbraker]

I thought we came to the conclusion that we should add new functions and not change ABI on the old ones. Yes this will mean copies of the functions but its only for a release.

Looks good otherwise.

[deadalnix]

So I marked the function that'd be messed up if passed LLVMAttribute in the future.

That'd be easy to rename the Instr family with a Call family functions. and make Argument explicit in the arguments ones (which would be a plus no matter what, the current names are bad). The only one that would suffer a bit is the LLVMAddFunctionAttr as they'd get a clunky name.

The amount of code breakage would be much greater, however, transition can be done faster (as the new API can be introduced right away, without need to wait of a deprecation cycle for LLVMAttribute).

My gut feeling is that the longer transition with less breakage is preferable. I had in mind, add this. Wait a release. Remove LLVMAttribute, so everybody has to move to LLVMGetAttrKindID at this point. Wait one more release, and start messing with attributes ids.

If we are going to duplicate the API, then returning LLVMAttribute from LLVMGetAttrKindID is not really necessary anymore, we can go straight to the desired end result, but obviously, all code using attributes needs to be changed client side, not just adding a call to retrieve the attribute id.

@echristo , @whitequark , what are your thoughts ?

include/llvm-c/Core.h

1953

LLVMAddFunctionAttr

2082

LLVMAddAttribute

2089

LLVMRemoveAttribute

2553

LLVMAddInstrAttribute

2555

LLVMRemoveInstrAttribute

[deadalnix]

Ok Talking with @whitequark , we will just revamp the whole thing and map attributes objects (but not attribute sets). This is completely incompatible with the current apporach, so that solve the problem of knowing how much we deprecate and how fast.

[deadalnix]

Revamp the whole thing as per discussion with @whitequark . This is fairly incomplete, but hopefully should allow to discuss where this is going.

The new plan is to Add a LLVMAttributeRef type and add an API to crate these attributes. Then we have a set of functions for functions/parameters/calls to set/get these attributes.

Right now, only the Return functions are made, so that we can discuss the details without having too much code out there. Once design is settled, we can add other attributes.

[whitequark]

One thing I would like to have right away is a way to get the kind of an attribute. Looks good otherwise.

[deadalnix]

Add a way to query attribute kind.

[deadalnix]

Some renaming

[whitequark]

include/llvm-c/Core.h

492

Document this function?

[deadalnix]

Add doc for LLVMGetAttributeKind

[whitequark]

I have no more objections.

[jyknight]

Seems to me it'd be cleaner to actually expose the set of attributes. Here's the API I'd propose:

typedef unsigned LLVMAttrKind;

/* Returns the number corresponding to a given built-in attribute. */
LLVMAttrKind LLVMGetAttributeKindForName(const char *Name);

/* In the below APIs, "Index" shall refer either to the parameter number (1 to N),
   LLVMAttributeReturnIndex, or LLVMAttributeFunctionIndex. */
enum { 
  LLVMAttributeReturnIndex = 0U,
  LLVMAttributeFunctionIndex = ~0U
};


/* Retrieve and replace attribute sets. These work on CallInst, InvokeInst, and Function values. */
LLVMAttributeSetRef LLVMGetAttrs(LLVMValueRef Val);
void LLVMSetAttrs(LLVMValueRef Val, LLVMAttributeSetRef AS);

/* Retrieve the number of attributes in a set */
unsigned LLVMASNumAttributes(LLVMAttributeSetRef AS, unsigned Index);

/* Whether the attr kind of the given attribute is a String, not an LLVMAttrKind enum */
bool LLVMASAttrIsStringKindAtIndex(LLVMAttributeSetRef AS, unsigned Index, unsigned i);
/* Retrieve the kind of attribute at a given index. Returns 0 on failure, e.g. if that attribute is a string attribute. */
LLVMAttrKind LLVMASGetKindAtIndex(LLVMAttributeSetRef AS, unsigned Index, unsigned i);
/* Retrieve the kind string of a string attribute. Returns NULL on failure, e.g. if that attribute is of an LLVMAttrKind enum */
char *LLVMASGetStringKindAtIndex(LLVMAttributeSetRef AS, unsigned Index, unsigned i);

/* Query if a given attribute kind is present */
LLVMBool LLVMASHasAttribute(LLVMAttributeSetRef AS, unsigned Index, LLVMAttrKind Kind);

/* Retrieve the value of an integer attribute. Fails if the given kind doesn't store an integer */
uint64_t LLVMASGetIntAttribute(LLVMAttributeSetRef AS, unsigned Index, LLVMAttrKind Kind);
/* Retrieve the value of a string attribute; if not present, returns NULL */
char *LLVMASGetStringAttribute(LLVMAttributeSetRef AS, unsigned Index, char *Name);

/* Note: all setters return a new LLVMAttributeSetRef, they do not mutate the argument in place. */

LLVMAttributeSetRef LLVMASSetAttribute(LLVMAttributeSetRef AS, unsigned Index, LLVMAttrKind Kind);
LLVMAttributeSetRef LLVMASSetIntAttribute(LLVMAttributeSetRef AS, unsigned Index, LLVMAttrKind Kind, uint64_t Val);
LLVMAttributeSetRef LLVMASSetStringAttribute(LLVMAttributeSetRef AS, char *Name, char *Val);

LLVMAttributeSetRef LLVMASRemoveAttribute(LLVMAttributeSetRef AS, unsigned Index, LLVMAttrKind Kind);
LLVMAttributeSetRef LLVMASRemoveStringAttribute(LLVMAttributeSetRef AS, unsigned Index, char *Name);

LLVMAttributeSetRef LLVMCreateEmptyAS(LLVMContextRef Ctx);

I think that API is implementable except for one detail: empty AttributeSets don't actually store a Context right now, so all the mutating operations would need an extra LLVMContext argument, in case the input LLVMAttributeSetRef is empty. (But, I think that could/should be easily fixed, for a marginally nicer C++ API, too.)

[deadalnix]

@jyknight The proposed API do not allow for the different kind of attributes to be mapped, contrary to what is proposed here. I think @whitequark made a good case for why we should map the attribute themselves, but I think going for the attribute set itself is overkill.

[jyknight]

In D19181#413393, @deadalnix wrote:

@jyknight The proposed API do not allow for the different kind of attributes to be mapped, contrary to what is proposed here.

I'm not sure what you mean? I proposed accessors for the different types of attributes that exist now: flag, int, and string.

[whitequark]

@jyknight I strongly dislike exposing AttributeSet. What is the proposed usecase? FFI calls are expensive and using AttributeSet requires at least three operations (RMW) instead of one (Add) for the most common case (adding a single attribute). Further, AttributeSet is not easily interrogated; to get the full list of attributes, I propose (in the future) to add a function that returns an array of LLVMAttributeRef, which is again much nicer to bindings.

[jyknight]

In D19181#413618, @whitequark wrote:

@jyknight I strongly dislike exposing AttributeSet. What is the proposed usecase? FFI calls are expensive and using AttributeSet requires at least three operations (RMW) instead of one (Add) for the most common case (adding a single attribute). Further, AttributeSet is not easily interrogated; to get the full list of attributes, I propose (in the future) to add a function that returns an array of LLVMAttributeRef, which is again much nicer to bindings.

Why do you say adding a single attribute is the most common use-case? I'd expect the most common use to be to be creating a function definition/declaration/call instruction from scratch, with a given set of specified function/parameter/return-value attributes. Which is ideally suited for constructing an attribute set, and then calling a setter to place it onto the instruction. Why do you think RMW operations would be primary?

I'm not sure what you mean by not easily interrogated. In my proposed API, you'd have a loop, from 0 to what LLVMASNumAttributes returns, and can call LLVMASGetKindAtIndex and LLVMASGetStringKindAtIndex for each value, to retrieve them. That seems easy enough, and perfectly nice for bindings?

Actually, if we're really concerned about efficiency, we should also wrap AttrBuilder, and a function to construct an AttributeSet from an array of AttrBuilders, so that all the intermediate steps along the path towards building the AttributeSet you want in the end don't need to be memoized in the context.

[deadalnix]

@jyknight exposing thing via the C API is always a liability. The C API cannot be changed as easily as the C++ API. There are various reasons for that, the most important one being that people are using the C API from other languages than C, as binding to C is easier than C++.

I don't think exposing AttrBuilder is a wise move. I don't think exposing the attribute set is either. This is not a super convenient API to begin with, proof is the amount of accessors that wrap it all over the place in the C++ code. I'll have to side with @whitequark here.

[jyknight]

In D19181#414320, @deadalnix wrote:

I don't think exposing AttrBuilder is a wise move. I don't think exposing the attribute set is either. This is not a super convenient API to begin with, proof is the amount of accessors that wrap it all over the place in the C++ code. I'll have to side with @whitequark here.

I agree, let's not add AttrBuilder unless there was shown a real need for it. But then let's not talk about efficiency as a primary goal here, either.

To me, a minimal yet usable and fully functional API seems like it should be the goal.

So, I don't understand why you'd expose LLVMAttributeRef. That actually feels much more like an internal implementation detail than the AttributeSet as a whole. I kind of like exposing the AttributeSet, as it's a reusable object which you can easily assign to multiple instructions without rebuilding from scratch each time.

However, if we don't want to expose AttributeSet (which, ok, perhaps is the right call here), I'd make some small changes to my proposal:

1. Remove LLVMGetAttrs and LLVMSetAttrs.
2. Modify all of the functions "LLVMAS*" that took an LLVMAttributeSetRef, to instead take an LLVMValueRef of the function or call instruction directly. (and rename the functions to something more appropriate).

That leaves, most importantly, some properties of the original proposal:

• The attribute location, function vs return vs param, is a parameter, not part of the function name.
• The same function works on functions and call/invoke instructions, which after all, are both exposed as LLVMValueRef.
• All types (string, int, on/off flag) of attributes are accessible in all locations.

[whitequark]

@jyknight

Why do you say adding a single attribute is the most common use-case? I'd expect the most common use to be to be creating a function definition/declaration/call instruction from scratch, with a given set of specified function/parameter/return-value attributes. Which is ideally suited for constructing an attribute set, and then calling a setter to place it onto the instruction. Why do you think RMW operations would be primary?

Based on the way bindings are implemented, and compatibility requirements, I expect that the majority of bindings will wish to stay compatible with code using their existing interface, which means adding attributes one by one. The interface that I proposed (LLVMAttributeRef + array getters/setters) allows them to painlessly upgrade from the old interface to the new one, and once they are ready to set attributes in bulk, much like AttributeSet works, they can easily opt in by using the array setters.

Whereas with your proposal, a significant overhaul of existing code is required to stay at the same place wrt amount of FFI calls.

Having the attribute position as a parameter is a minor convenience for LLVM-C maintainers. For bindings, both APIs are equally laborous to use but the one you propose also requires duplicating a enum within the binding code. So, I have no strong opinion of it but I don't see much point either.

[jyknight]

@whitequark: I think you have a picture in your mind of what the final state will look like that could better justify the choices made in this patch, than the couple of APIs added here do. And I'm willing to imagine that it could be better than my proposal. But, without a complete concrete API proposal being written down, it's not easy to come to that shared understanding.

So, like I said before to @deadalnix a while ago, I'd really like to see the complete proposed API for attribute access, even if an initial code patch doesn't implement the whole thing yet. Can one of you write that up, similar to how I did?

[Wallbraker]

I kind of prefer the AttributeSet approach, but don't feel to strongly about it.

Creating immutable state objects up front and then reusing it will always be faster then individually setting state on objects. Also AttributeSets better reflect what you see when you read the LLVM bitcode textual representation.

Should AttributeSet go away we can keep a copy around in the C interface.

But I'm fine with either, tho I mirror @jyknight comment about seeing the complete interface, even if its just the header changes.

[deadalnix]

In D19181#415126, @Wallbraker wrote:

I kind of prefer the AttributeSet approach, but don't feel to strongly about it.

Creating immutable state objects up front and then reusing it will always be faster then individually setting state on objects. Also AttributeSets better reflect what you see when you read the LLVM bitcode textual representation.

AttributeSet does map all the attribute on a function/call, including return and parameters. There is no way you can create one and reuse it in the general case. Reading your comment, I'm not sure you are familiar with the AttributeSet thing in C++ .

[deadalnix]

@jyknight Complete API is nothing more than a set of getter/setter for function/parameters/return/calls and LLVMCreateAttribute to create attribute. We can add variation of LLVMCreateAttribute as new kind of attribute appear (for instance, target dependent ones).

[jyknight]

In D19181#418165, @deadalnix wrote:

@jyknight Complete API is nothing more than a set of getter/setter for function/parameters/return/calls and LLVMCreateAttribute to create attribute.

Then this should be really easy to describe....I'm not sure why you keep pushing back on this.

[deadalnix]

I'm not sure what more you expect as description. The very thing you quote state the intent. Do I need to detail what a setter and/or a getter is ?

[jyknight]

In D19181#419245, @deadalnix wrote:

I'm not sure what more you expect as description. The very thing you quote state the intent. Do I need to detail what a setter and/or a getter is ?

I want to see a concrete API that covers everything needed as it would appear in the header file (at least approximately), similar to what I wrote after "Here's the API I'd propose:" a few comments back. Not just general intentions.

[deadalnix]

You get it for return :

void LLVMAddReturnAttr(LLVMValueRef Fn, LLVMAttributeRef A);
void LLVMRemoveReturnAttr(LLVMValueRef Fn, unsigned KindID);
LLVMAttributeRef LLVMGetReturnAttr(LLVMValueRef Fn, unsigned KindID);

Then the same for calls, functions and parameters.

[deadalnix]

Ping

[jyknight]

I'm still hoping for a full response to my previous question, as the
previous response is clearly not a complete attribute access API. (Or, if
it is supposed to be, is clearly insufficient...)

[deadalnix]

Listen, I have no idea what you want. There are the function required to create an attribute in this diff, and there is a plan for accessors. Nothing more is needed. I've asked already what you wanted and you never gave an answer. Either somethign is missing and you can point at it, or we move forward here.

[jyknight]

I'd like to see a complete list of the API functions that will be created. Recall the proposal I wrote earlier, which contained declarations that could go in a header. Write one just like that, containing all of the functions you propose to create for attribute access. That is, don't say "Then the same for calls, functions and parameters." or "and there is a plan for accessors" -- that's too vague for me to fully understand your intent.

To maybe help get things going, here's my previous proposal after modifying it to remove AttributeSet (as I'd suggested earlier). Please respond with a counter-proposal along the same lines.

/* All attribute accessors work on CallInst, InvokeInst, and Function values. Use on other kinds of
 * LLVMValueRef objects is an error. */

typedef unsigned LLVMAttrKind;

/* Returns the number corresponding to a given built-in attribute. */
LLVMAttrKind LLVMGetAttributeKindForName(const char *Name);

/* In the below APIs, "Index" shall refer either to the parameter number, numbered 1 to N,
   or LLVMAttributeReturnIndex or LLVMAttributeFunctionIndex. */
enum {
  LLVMAttributeReturnIndex = 0U,
  LLVMAttributeFunctionIndex = ~0U
};

/* Retrieve the number of attributes in the attribute-set. For use with the 'LLVM*AtIndex' APIs
 * below.*/
unsigned LLVMNumAttributes(LLVMValueRef AS, unsigned Index);

/* Whether the attr kind of the given attribute is a String, not an LLVMAttrKind enum */
bool LLVMIsStringAttributeAtIndex(LLVMValueRef AS, unsigned Index, unsigned i);

/* Retrieve the kind of attribute at a given index. Returns 0 on failure, e.g. if that attribute is
 * a string attribute. */
LLVMAttrKind LLVMGetAttributeKindAtIndex(LLVMValueRef AS, unsigned Index, unsigned i);

/* Retrieve the kind string of a string attribute. Returns NULL on failure, e.g. if that attribute
 * is of an LLVMAttrKind enum */
char *LLVMGetStringAttributeKindAtIndex(LLVMValueRef AS, unsigned Index, unsigned i);


/* Query if a given attribute kind is present */
LLVMBool LLVMHasAttribute(LLVMValueRef AS, unsigned Index, LLVMAttrKind Kind);

/* Retrieve the value of an integer attribute. Fails if the given kind isn't present or doesn't
 * store an integer. */
uint64_t LLVMGetIntAttribute(LLVMValueRef AS, unsigned Index, LLVMAttrKind Kind);

/* Retrieve the value of a string attribute; if not present, returns NULL. */
char *LLVMGetStringAttribute(LLVMValueRef AS, unsigned Index, char *Name);

/* Set/replace attributes. */
void LLVMSetAttribute(LLVMValueRef AS, unsigned Index, LLVMAttrKind Kind);
void LLVMSetIntAttribute(LLVMValueRef AS, unsigned Index, LLVMAttrKind Kind, uint64_t Val);
void LLVMSetStringAttribute(LLVMValueRef AS, char *Name, char *Val);

/* Removing attributes. */
void LLVMRemoveAttribute(LLVMValueRef AS, unsigned Index, LLVMAttrKind Kind);
void LLVMRemoveStringAttribute(LLVMValueRef AS, unsigned Index, char *Name);

[deadalnix]

By the power of cutting and pasting, I present you :

typedef struct LLVMOpaqueAttributeRef *LLVMAttributeRef;
unsigned LLVMGetAttributeKindForName(const char *Name, size_t SLen);
LLVMAttributeRef LLVMCreateAttribute(LLVMContextRef C, unsigned KindID);

void LLVMAddReturnAttr(LLVMValueRef Fn, LLVMAttributeRef A);
void LLVMRemoveReturnAttr(LLVMValueRef Fn, unsigned KindID);
LLVMAttributeRef LLVMGetReturnAttr(LLVMValueRef Fn, unsigned KindID);

void LLVMAddCallSiteAttr(LLVMValueRef Fn, LLVMAttributeRef A);
void LLVMRemoveCallSiteAttr(LLVMValueRef Fn, unsigned KindID);
LLVMAttributeRef LLVMGetCallSiteAttr(LLVMValueRef Fn, unsigned KindID);

void LLVMAddFunctionAttr(2LLVMValueRef Fn, LLVMAttributeRef A);
void LLVMRemoveFunctionAttr2(LLVMValueRef Fn, unsigned KindID);
LLVMAttributeRef LLVMGetFunctionAttr(LLVMValueRef Fn, unsigned KindID);

void LLVMAddParamAttr(LLVMValueRef Fn, LLVMAttributeRef A);
void LLVMRemoveParamAttr(LLVMValueRef Fn, unsigned KindID);
LLVMAttributeRef LLVMGetParamAttr(LLVMValueRef Fn, unsigned KindID);

[jyknight]

Thanks! Now that I can actually see your proposal, I can respond to it.

There's some things missing:

• No way to enumerate which attributes are present.
• No way to access or create integer-valued attributes.
• No way to access or create string attributes.

The LLVMGetCallSiteAttr (and related) function is missing an "index" parameter -- at least I assume you mean for there to be such a parameter, like the old LLVMGetInstrAttribute?

If so: why does it make sense to have one function that handles all of function, return, and param attributes for call instructions, depending on the value of an index parameter, but to use three functions to do the same for declarations/definitions?

If not: can you clarify what the intended usage of the function is?

[deadalnix]

In D19181#424905, @jyknight wrote:

There's some things missing:

• No way to enumerate which attributes are present.

True. That's not the most useful use case in practice, but it can be added.

• No way to access or create integer-valued attributes.

That's why LLVMAttributeRef needs to be exposed. All is needed to support new kind of attribute is a new builder, and then the same accessors can be used to set them.

• No way to access or create string attributes.

Same as for int attributes + They need a new getter.

The LLVMGetCallSiteAttr (and related) function is missing an "index" parameter -- at least I assume you mean for there to be such a parameter, like the old LLVMGetInstrAttribute?

If so: why does it make sense to have one function that handles all of function, return, and param attributes for call instructions, depending on the value of an index parameter, but to use three functions to do the same for declarations/definitions?

If not: can you clarify what the intended usage of the function is?

Yes they need one extra index parameter, sorry.

void LLVMAddCallSiteAttr(LLVMValueRef Fn, unsigned Idx, LLVMAttributeRef A);
void LLVMRemoveCallSiteAttr(LLVMValueRef Fn, unsigned Idx, unsigned KindID);
LLVMAttributeRef LLVMGetCallSiteAttr(LLVMValueRef Fn, unsigned Idx, unsigned KindID);

An alternative is to support both function and callsites in the 3 other sets of setters/getters and get rid of that one.

[jyknight]

For avoidance of doubt, the current status of this, from my point-of-view, is that I'd like to see the new API after adding those missing items, followed by an explanation of why the result is preferable to my proposal.

Either that, or we can go at this from the other direction: starting with my last proposal, suggest modifications or describe why it's not a good way to go.

[deadalnix]

To be able to map the different kind of attribute, one does need to expose LLVMAttributeRef or alike. You proposal doesn't allow for this.

On exposing the AttributeSet, I'd rather avoid exposing something that is not strictly necessary as it may end up evolve in the future. In addition, a ton of code wrap AttributeSet and use setter/getter in the C++ codebase, which should be a sign that this isn't such a great abstraction APIwise.

[jyknight]

In D19181#434323, @deadalnix wrote:

To be able to map the different kind of attribute, one does need to expose LLVMAttributeRef or alike. You proposal doesn't allow for this.

Please explain. The API I proposed does allow access to all three of the kinds of attributes.

On exposing the AttributeSet, I'd rather avoid exposing something that is not strictly necessary as it may end up evolve in the future. In addition, a ton of code wrap AttributeSet and use setter/getter in the C++ codebase, which should be a sign that this isn't such a great abstraction APIwise.

The last proposal from me had no AttributeSet.

[deadalnix]

So I gave some more time to think to this and the conclusion I come with is that we should take @jyknight 's idea to expose LLVMAttributeReturnIndex and LLVMAttributeFunctionIndex and provide 2 sets of accessors : for function and for CallSite.

On the other hand, I think this is the right thing to do to expose LLVMAttributeRef as a common abstraction for attributes. This way, only constructors and single attribute getters needs to have a version for each attribute kind. Setters and getters for all attributes (needed for binding that have expensive C function calls) can be agnostic of the actual kind of attribute.

Thought ?

[deadalnix]

Sample of the current proposal. Also pingpingping !

[Wallbraker]

I would like to see something that adds the integer and string attributes that @jyknight brought up, so we know they work before we commit to this API.

include/llvm-c/Core.h

388–391

This will be a really annoying off by one situation, how are arguments addressed in other parts of the API, if they start with zero so should this.

Make function and return be -1 and -2.

[deadalnix]

There is no such thing as int attributes. There are attributes with int parameters, which is different. These are irrelevant as far as setting/getting attributes goes.

include/llvm-c/Core.h

388–391

This is how it works on the C++ side, and is unlikely to change anytime soon.

[deadalnix]

Add support for string attribute.

[deadalnix]

Add echo test

[whitequark]

include/llvm-c/Core.h

388–391

I agree that, since the C and C++ APIs don't share this enum anyway, the numbering should be rectified as @Wallbraker says.

513

Does this allocate? If yes, needs to be mentioned in the docstring I think (etc etc LLVMDisposeString etc etc)

tools/llvm-c-test/echo.cpp

800 ↗

(On Diff #60436)

No. This is an incredibly inefficient way to iterate through attributes. A consumer of the C API that attempts to read bitcode would have to do what, at least ~50 FFI calls per function *or* call site just to discover attributes? This will not do.

[deadalnix]

include/llvm-c/Core.h

513

It doesn't.

tools/llvm-c-test/echo.cpp

800 ↗

(On Diff #60436)

We discussed that, I'll add getter to get attributes en masse. Can we go incremental here ? This already provides more than the old C API. There is nothing blocking the addition of :

LLVMGetAttributeCountAtIndex(F, Idx);
LLVMGetAttributesAtIndex(G, Idx, LLVMAttributeRef *Buf);

Many accessor in the C have the 2 version, and that's fine. Note that the second version require to allocate, which isn't always better, so providing both is fine.

Also, this really doesn't matter as this is not what people commonly do with attribute (unless you often clone module manually ?).

[whitequark]

include/llvm-c/Core.h

513

Needs a mention anyway (since most other functions do allocate).

tools/llvm-c-test/echo.cpp

800 ↗

(On Diff #60436)

LLVMGetEndAttrKind() is an API that should not exist; it solves a problem that shouldn't be there in the first place. The cost of making 50 (and growing) FFI calls is much higher than the cost of a single stack allocation. Also, mass getters are not a lot of work.

Also, this really doesn't matter as this is not what people commonly do with attribute (unless you often clone module manually ?)

Define "commonly". What you suggest would make reading bitcode and transforming it into some language-specific representation of IR needlessly slow. This actually happens in practice, e.g. Haskell's LLVM-General bindings have a pure representation of IR.

[deadalnix]

tools/llvm-c-test/echo.cpp

800 ↗

(On Diff #60436)

Common use cases include setting a given attribute or checking if an attribute is set. There is almost no code that want to get the whole set of attribute, passed code that dump/serialize IR somehow. The use case exists, but is not what is commonly done.

The current API is superior for both these use case. Look at what your code is doing and tell me how often do you want to have all attributes served to you ? In addition consider you'll have to call the LLVMIsXXXAttribute family of function anyway when getting attribute en masse as you won't know what kind they are. If function calls are the #1 concern, this may not help at all.

Also, while I understand that FFI is expensive, I happen (and @Wallbraker as well) to have the opposite "cost structure" where calls are cheap but allocation aren't.

[deadalnix]

include/llvm-c/Core.h

513

We have done it the other way around when it allocate in the past. Function that allocate mention what dispose function you need to call to free the string. Most function returning a string do not transfers ownership.

[whitequark]

include/llvm-c/Core.h

499

What does this do if a target-dependent attribute is passed? This needs to be documented.

tools/llvm-c-test/echo.cpp

800 ↗

(On Diff #60436)

The use case exists, but is not what is commonly done.

Look at what your code is doing and tell me how often do you want to have all attributes served to you ?

And when it needs to be done, you unnecessarily pessimize it.

The current API is superior for both these use case.

Sure. I do not propose altering any of the APIs related to these two use cases.

In addition consider you'll have to call the LLVMIsXXXAttribute family of function anyway when getting attribute en masse as you won't know what kind they are.

Only once per an attribute that 1) actually exists 2) was not previously encountered, as any such system would use a hashtable indexed by LLVMAttributeRef 3) is target-dependent (so that LLVMGetAttributeKindAsEnum doesn't work), which is a very small minority of attributes. So this isn't expensive at all, and in most cases only involves running the mass getter.

allocation aren't

You don't have stack allocation?

[deadalnix]

include/llvm-c/Core.h

499

It does the same thing as when you try to call getKindAsEnum on a target dependent attribute, meaning an assertion failure if enabled, garbage if not.

tools/llvm-c-test/echo.cpp

800 ↗

(On Diff #60436)

Only once per an attribute that 1) actually exists 2) was not previously encountered, as any such system would use a hashtable indexed by LLVMAttributeRef 3) is target-dependent (so that LLVMGetAttributeKindAsEnum doesn't work), which is a very small minority of attributes. So this isn't expensive at all, and in most cases only involves running the mass getter.

1 yes, 2 you'll have to build yourself and 3 certainly not. How do you know they are target-dependent without calling these functions ?

You don't have stack allocation?

Not of size that vary at runtime (at least not without building some scafolding).

[whitequark]

As per IRC discussion, some renaming

include/llvm-c/Core.h

487

LLVMGetLastAttributeKind

504

LLVMCreateStringAttribute

518

LLVMIsFlagAttribute?

[deadalnix]

include/llvm-c/Core.h

518

OK sooooooooo. As per the way things work, this will be true on all attribute that aren't flag but have the value 0 (this is how the code works under the hood).

Not sure what is the best path forward here, but Flag doesn't seems to be a good name.

[deadalnix]

Various name update.

[deadalnix]

Aggregate getters go into D21265

[deadalnix]

LLVMGetLastAttributeKind

[deadalnix]

Various updates as per discussion with @whitequark

[deadalnix]

Update release notes

bindings/go/llvm/ir.go

func (v Value) Params() []Value {
   return out
 }
 func (v Value) Param(i int) (rv Value)  { rv.C = C.LLVMGetParam(v.C, C.unsigned(i)); return }
 func (v Value) ParamParent() (rv Value) { rv.C = C.LLVMGetParamParent(v.C); return }
 func (v Value) FirstParam() (rv Value)  { rv.C = C.LLVMGetFirstParam(v.C); return }
 func (v Value) LastParam() (rv Value)   { rv.C = C.LLVMGetLastParam(v.C); return }
 func NextParam(v Value) (rv Value)      { rv.C = C.LLVMGetNextParam(v.C); return }
 func PrevParam(v Value) (rv Value)      { rv.C = C.LLVMGetPreviousParam(v.C); return }
-func (v Value) AddAttribute(a Attribute) {
-  if a >= 1<<32 {
-    panic("attribute value currently unsupported")
-  }
-  C.LLVMAddAttribute(v.C, C.LLVMAttribute(a))
-}
-func (v Value) RemoveAttribute(a Attribute) {
-  if a >= 1<<32 {
-    panic("attribute value currently unsupported")
-  }
-  C.LLVMRemoveAttribute(v.C, C.LLVMAttribute(a))
+func (v Value) AddAttribute(KindID uint) {
+  C.LLVMAddAttribute(v.C, C.uint(KindID))
+func (v Value) RemoveAttribute(KindID uint) {
+  C.LLVMRemoveAttribute(v.C, C.uint(KindID))
 }
 func (v Value) Attribute() Attribute        { return Attribute(C.LLVMGetAttribute(v.C)) }
 func (v Value) SetParamAlignment(align int) { C.LLVMSetParamAlignment(v.C, C.unsigned(align)) }
 
 // Operations on basic blocks
 func (bb BasicBlock) AsValue() (v Value)      { v.C = C.LLVMBasicBlockAsValue(bb.C); return }
 func (v Value) IsBasicBlock() bool            { return C.LLVMValueIsBasicBlock(v.C) != 0 }
 func (v Value) AsBasicBlock() (bb BasicBlock) { bb.C = C.LLVMValueAsBasicBlock(v.C); return }

 
 // Operations on call sites
 func (v Value) SetInstructionCallConv(cc CallConv) {
   C.LLVMSetInstructionCallConv(v.C, C.unsigned(cc))
 }
 func (v Value) InstructionCallConv() CallConv {
   return CallConv(C.LLVMCallConv(C.LLVMGetInstructionCallConv(v.C)))
 }
-func (v Value) AddInstrAttribute(i int, a Attribute) {
-  if a >= 1<<32 {
-    panic("attribute value currently unsupported")
-  }
-  C.LLVMAddInstrAttribute(v.C, C.unsigned(i), C.LLVMAttribute(a))
-}
-func (v Value) RemoveInstrAttribute(i int, a Attribute) {
-  if a >= 1<<32 {
-    panic("attribute value currently unsupported")
-  }
-  C.LLVMRemoveInstrAttribute(v.C, C.unsigned(i), C.LLVMAttribute(a))
+func (v Value) AddInstrAttribute(i int, KindID uint) {
+  C.LLVMAddInstrAttribute(v.C, C.unsigned(i), C.uint(KindID))
+func (v Value) RemoveInstrAttribute(i int, KindID uint) {
+  C.LLVMRemoveInstrAttribute(v.C, C.unsigned(i), C.uint(KindID))
 }
 func (v Value) SetInstrParamAlignment(i int, align int) {
   C.LLVMSetInstrParamAlignment(v.C, C.unsigned(i), C.unsigned(align))
 }
 
 // Operations on call instructions (only)
 func (v Value) IsTailCall() bool    { return C.LLVMIsTailCall(v.C) != 0 }
 func (v Value) SetTailCall(is bool) { C.LLVMSetTailCall(v.C, boolToLLVMBool(is)) }

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 functions LLVMGetAttribute and LLVMGetFunctionAttr
+  are deprecated in favor of LLVMHasAttribute and LLVMHasFunctionAttr.
+
 * ``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

typedef enum {
     LLVMDSNote
 } LLVMDiagnosticSeverity;
 
 /**
  * @}
  */
 
 void LLVMInitializeCore(LLVMPassRegistryRef R);
 
 /** Deallocate and destroy all ManagedStatic variables.
     @see llvm::llvm_shutdown
     @see ManagedStatic */
 void LLVMShutdown(void);
 
 /*===-- Error handling ----------------------------------------------------===*/
 
 char *LLVMCreateMessage(const char *Message);
 void LLVMDisposeMessage(char *Message);
 
 /**

 /**
  * Return an unique id given an attribute name, or 0 if no attribute
  * by that name exists.
  *
  * NB: Attribute names and/or id are subject to change without
  * going through the C API deprecation cycle.
  */
 unsigned LLVMGetAttrKindID(const char *Name, size_t SLen);
 
 /**
  * @}
  */
 
 /**
  * @defgroup LLVMCCoreModule Modules
  *
  * Modules represent the top-level structure in an LLVM program. An LLVM
  * module is effectively a translation unit or a collection of
  * translation units merged together.
  *
  * @{
  */
 
 /**
  * Create a new, empty module in the global context.
  *
  * This is equivalent to calling LLVMModuleCreateWithNameInContext with
  * LLVMGetGlobalContext() as the context parameter.
  *
  * Every invocation should be paired with LLVMDisposeModule() or memory
  * will be leaked.
  */
 LLVMModuleRef LLVMModuleCreateWithName(const char *ModuleID);
 
 /**
  * Create a new, empty module in a specific context.
  *
  * Every invocation should be paired with LLVMDisposeModule() or memory
  * will be leaked.
  */
 LLVMModuleRef LLVMModuleCreateWithNameInContext(const char *ModuleID,
                                                 LLVMContextRef C);
 /**
  * Return an exact copy of the specified module.
  */
 LLVMModuleRef LLVMCloneModule(LLVMModuleRef M);
 
 /**

  *
  * @see llvm::Function::setGC()
  */
 void LLVMSetGC(LLVMValueRef Fn, const char *Name);
 
 /**
  * Add an attribute to a function.
  *
- * @see llvm::Function::addAttribute()
+ * @see llvm::Function::addFnAttr()
  */
-void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA);
+void LLVMAddFunctionAttr(LLVMValueRef Fn, unsigned KindID);
 
 /**
- * Add a target-dependent attribute to a function
+ * Add a target-dependent attribute to a function.
+ *
  * @see llvm::AttrBuilder::addAttribute()
+ * @see llvm::Function::addAttributes()
  */
 void LLVMAddTargetDependentFunctionAttr(LLVMValueRef Fn, const char *A,
                                         const char *V);
 
 /**
+ * Remove an attribute from a function.
+ */
+void LLVMRemoveFunctionAttr(LLVMValueRef Fn, unsigned KindID);
+
+/**
  * Obtain an attribute from a function.
  *
  * @see llvm::Function::getAttributes()
  */
 LLVMAttribute LLVMGetFunctionAttr(LLVMValueRef Fn);
 
 /**
- * Remove an attribute from a function.
+ * Check if a function has an attribute.
  */
-void LLVMRemoveFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA);
+LLVMBool LLVMHasFunctionAttr(LLVMValueRef Fn, unsigned KindID);
+
+/**
+ * Add a return attribute to a function.
+ */
+void LLVMAddReturnAttr(LLVMValueRef Fn, unsigned KindID);
+
+/**
+ * Remove a return attribute from a function.
+ */
+void LLVMRemoveReturnAttr(LLVMValueRef Fn, unsigned KindID);
+
+/**
+ * Check if a function's return has an attribute.
+ */
+LLVMBool LLVMHasReturnAttr(LLVMValueRef Fn, unsigned KindID);
 
 /**
  * @defgroup LLVMCCoreValueFunctionParameters Function Parameters
  *
  * Functions in this group relate to arguments/parameters on functions.
  *
  * Functions in this group expect LLVMValueRef instances that correspond
  * to llvm::Function instances.

  */
 LLVMValueRef LLVMGetPreviousParam(LLVMValueRef Arg);
 
 /**
  * Add an attribute to a function argument.
  *
  * @see llvm::Argument::addAttr()
  */
-void LLVMAddAttribute(LLVMValueRef Arg, LLVMAttribute PA);
+void LLVMAddAttribute(LLVMValueRef Arg, unsigned KindID);
 
 /**
  * Remove an attribute from a function argument.
  *
  * @see llvm::Argument::removeAttr()
  */
-void LLVMRemoveAttribute(LLVMValueRef Arg, LLVMAttribute PA);
+void LLVMRemoveAttribute(LLVMValueRef Arg, unsigned KindID);
 
 /**
  * Get an attribute from a function argument.
+ * This method is DEPRECATED.
  */
 LLVMAttribute LLVMGetAttribute(LLVMValueRef Arg);
 
 /**
+ * Checks if a function argument has a given attribute.
+ *
+ * @see llvm::Argument::hasAttribute()
+ */
+LLVMBool LLVMHasAttribute(LLVMValueRef Arg, unsigned KindID);
+
+/**
  * Set the alignment for a function parameter.
  *
  * @see llvm::Argument::addAttr()
  * @see llvm::AttrBuilder::addAlignmentAttr()
  */
 void LLVMSetParamAlignment(LLVMValueRef Arg, unsigned align);
 
 /**

  * This is the opposite of LLVMSetInstructionCallConv(). Reads its
  * usage.
  *
  * @see LLVMSetInstructionCallConv()
  */
 unsigned LLVMGetInstructionCallConv(LLVMValueRef Instr);
 
 
-void LLVMAddInstrAttribute(LLVMValueRef Instr, unsigned index, LLVMAttribute);
+void LLVMAddInstrAttribute(LLVMValueRef Instr, unsigned index,
+                           unsigned KindID);
 void LLVMRemoveInstrAttribute(LLVMValueRef Instr, unsigned index,
-                              LLVMAttribute);
+                              unsigned KindID);
 void LLVMSetInstrParamAlignment(LLVMValueRef Instr, unsigned index,
                                 unsigned align);
+LLVMBool LLVMHasInstrAttribute(LLVMValueRef Instr, unsigned index,
+                               unsigned KindID);
 
 /**
  * Obtain the pointer to the function invoked by this instruction.
  *
  * This expects an LLVMValueRef that corresponds to a llvm::CallInst or
  * llvm::InvokeInst.
  *
  * @see llvm::CallInst::getCalledValue()

lib/IR/Core.cpp

 void LLVMSetGC(LLVMValueRef Fn, const char *GC) {
   Function *F = unwrap<Function>(Fn);
   if (GC)
     F->setGC(GC);
   else
     F->clearGC();
 }
 
-void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA) {
+void LLVMAddFunctionAttr(LLVMValueRef Fn, unsigned KindID) {
   Function *Func = unwrap<Function>(Fn);
   const AttributeSet PAL = Func->getAttributes();
-  AttrBuilder B(PA);
+  AttrBuilder B((LLVMAttribute)KindID);
   const AttributeSet PALnew =
     PAL.addAttributes(Func->getContext(), AttributeSet::FunctionIndex,
                       AttributeSet::get(Func->getContext(),
                                         AttributeSet::FunctionIndex, B));
   Func->setAttributes(PALnew);
 }
 
 void LLVMAddTargetDependentFunctionAttr(LLVMValueRef Fn, const char *A,
                                         const char *V) {
   Function *Func = unwrap<Function>(Fn);
   AttributeSet::AttrIndex Idx =
     AttributeSet::AttrIndex(AttributeSet::FunctionIndex);
   AttrBuilder B;
 
   B.addAttribute(A, V);
   AttributeSet Set = AttributeSet::get(Func->getContext(), Idx, B);
   Func->addAttributes(Idx, Set);
 }
 
-void LLVMRemoveFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA) {
+void LLVMRemoveFunctionAttr(LLVMValueRef Fn, unsigned KindID) {
   Function *Func = unwrap<Function>(Fn);
   const AttributeSet PAL = Func->getAttributes();
-  AttrBuilder B(PA);
+  AttrBuilder B((LLVMAttribute)KindID);
   const AttributeSet PALnew =
     PAL.removeAttributes(Func->getContext(), AttributeSet::FunctionIndex,
                          AttributeSet::get(Func->getContext(),
                                            AttributeSet::FunctionIndex, B));
   Func->setAttributes(PALnew);
 }
 
 LLVMAttribute LLVMGetFunctionAttr(LLVMValueRef Fn) {
   Function *Func = unwrap<Function>(Fn);
   const AttributeSet PAL = Func->getAttributes();
   return (LLVMAttribute)PAL.Raw(AttributeSet::FunctionIndex);
 }
 
+LLVMBool LLVMHasFunctionAttr(LLVMValueRef Fn, unsigned KindID) {
+  return LLVMGetFunctionAttr(Fn) | KindID;
+}
+
+void LLVMAddReturnAttr(LLVMValueRef Fn, unsigned KindID) {
+  Function *Func = unwrap<Function>(Fn);
+  const AttributeSet PAL = Func->getAttributes();
+  AttrBuilder B((LLVMAttribute)KindID);
+  const AttributeSet PALnew =
+    PAL.addAttributes(Func->getContext(), AttributeSet::ReturnIndex,
+                      AttributeSet::get(Func->getContext(),
+                                        AttributeSet::FunctionIndex, B));
+  Func->setAttributes(PALnew);
+}
+
+void LLVMRemoveReturnAttr(LLVMValueRef Fn, unsigned KindID) {
+  Function *Func = unwrap<Function>(Fn);
+  const AttributeSet PAL = Func->getAttributes();
+  AttrBuilder B((LLVMAttribute)KindID);
+  const AttributeSet PALnew =
+    PAL.removeAttributes(Func->getContext(), AttributeSet::FunctionIndex,
+                         AttributeSet::get(Func->getContext(),
+                                           AttributeSet::FunctionIndex, B));
+  Func->setAttributes(PALnew);
+}
+
+LLVMBool LLVMHasReturnAttr(LLVMValueRef Fn, unsigned KindID) {
+  Function *Func = unwrap<Function>(Fn);
+  const AttributeSet PAL = Func->getAttributes();
+  return PAL.Raw(AttributeSet::FunctionIndex) | KindID;
+}
+
 /*--.. Operations on parameters ............................................--*/
 
 unsigned LLVMCountParams(LLVMValueRef FnRef) {
   // This function is strictly redundant to
   //   LLVMCountParamTypes(LLVMGetElementType(LLVMTypeOf(FnRef)))
   return unwrap<Function>(FnRef)->arg_size();
 }
 

 LLVMValueRef LLVMGetPreviousParam(LLVMValueRef Arg) {
   Argument *A = unwrap<Argument>(Arg);
   Function::arg_iterator I(A);
   if (I == A->getParent()->arg_begin())
     return nullptr;
   return wrap(&*--I);
 }
 
-void LLVMAddAttribute(LLVMValueRef Arg, LLVMAttribute PA) {
+void LLVMAddAttribute(LLVMValueRef Arg, unsigned KindID) {
   Argument *A = unwrap<Argument>(Arg);
-  AttrBuilder B(PA);
+  AttrBuilder B((LLVMAttribute)KindID);
   A->addAttr(AttributeSet::get(A->getContext(), A->getArgNo() + 1,  B));
 }
 
-void LLVMRemoveAttribute(LLVMValueRef Arg, LLVMAttribute PA) {
+void LLVMRemoveAttribute(LLVMValueRef Arg, unsigned KindID) {
   Argument *A = unwrap<Argument>(Arg);
-  AttrBuilder B(PA);
+  AttrBuilder B((LLVMAttribute)KindID);
   A->removeAttr(AttributeSet::get(A->getContext(), A->getArgNo() + 1,  B));
 }
 
 LLVMAttribute LLVMGetAttribute(LLVMValueRef Arg) {
   Argument *A = unwrap<Argument>(Arg);
   return (LLVMAttribute)A->getParent()->getAttributes().
     Raw(A->getArgNo()+1);
 }
 
+LLVMBool LLVMHasAttribute(LLVMValueRef Arg, unsigned KindID) {
+  return LLVMGetAttribute(Arg) | KindID;
+}
 
 void LLVMSetParamAlignment(LLVMValueRef Arg, unsigned align) {
   Argument *A = unwrap<Argument>(Arg);
   AttrBuilder B;
   B.addAlignmentAttr(align);
   A->addAttr(AttributeSet::get(A->getContext(),A->getArgNo() + 1, B));
 }
 

 }
 
 void LLVMSetInstructionCallConv(LLVMValueRef Instr, unsigned CC) {
   return CallSite(unwrap<Instruction>(Instr))
     .setCallingConv(static_cast<CallingConv::ID>(CC));
 }
 
 void LLVMAddInstrAttribute(LLVMValueRef Instr, unsigned index,
-                           LLVMAttribute PA) {
+                           unsigned KindID) {
   CallSite Call = CallSite(unwrap<Instruction>(Instr));
-  AttrBuilder B(PA);
+  AttrBuilder B((LLVMAttribute)KindID);
   Call.setAttributes(
     Call.getAttributes().addAttributes(Call->getContext(), index,
                                        AttributeSet::get(Call->getContext(),
                                                          index, B)));
 }
 
 void LLVMRemoveInstrAttribute(LLVMValueRef Instr, unsigned index,
-                              LLVMAttribute PA) {
+                              unsigned KindID) {
   CallSite Call = CallSite(unwrap<Instruction>(Instr));
-  AttrBuilder B(PA);
+  AttrBuilder B((LLVMAttribute)KindID);
   Call.setAttributes(Call.getAttributes()
                        .removeAttributes(Call->getContext(), index,
                                          AttributeSet::get(Call->getContext(),
                                                            index, B)));
 }
 
+LLVMBool LLVMHasInstrAttribute(LLVMValueRef Instr, unsigned index,
+                               unsigned KindID) {
+  CallSite Call = CallSite(unwrap<Instruction>(Instr));
+  const AttributeSet PAL = Call.getAttributes();
+  return PAL.Raw(index) | KindID;
+}
+
 void LLVMSetInstrParamAlignment(LLVMValueRef Instr, unsigned index,
                                 unsigned align) {
   CallSite Call = CallSite(unwrap<Instruction>(Instr));
   AttrBuilder B;
   B.addAlignmentAttr(align);
   Call.setAttributes(Call.getAttributes()
                        .addAttributes(Call->getContext(), index,
                                       AttributeSet::get(Call->getContext(),