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

2004

LLVMAddFunctionAttr

2119

LLVMAddAttribute

2126

LLVMRemoveAttribute

2582

LLVMAddInstrAttribute

2583

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

510

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.

531

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

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

531

It doesn't.

tools/llvm-c-test/echo.cpp

800

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

531

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

tools/llvm-c-test/echo.cpp

800

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

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

531

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

517

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

tools/llvm-c-test/echo.cpp

800

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

517

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

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

505

LLVMGetLastAttributeKind

522

LLVMCreateStringAttribute

536

LLVMIsFlagAttribute?

[deadalnix]

include/llvm-c/Core.h

536

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

include/llvm-c/Core.h

 typedef enum {
     LLVMDSError,
     LLVMDSWarning,
     LLVMDSRemark,
     LLVMDSNote
 } LLVMDiagnosticSeverity;
 
 /**
+ * Attribute index are either LLVMAttributeReturnIndex,
+ * LLVMAttributeFunctionIndex or a parameter number from 1 to N.
+ */
+enum {
+  LLVMAttributeReturnIndex = 0U,
+  // ISO C restricts enumerator values to range of 'int'
+  // (4294967295 is too large)
+  // LLVMAttributeFunctionIndex = ~0U,
+  LLVMAttributeFunctionIndex = -1,
+};
+
+typedef unsigned LLVMAttributeIndex;
+
+/**
  * @}
  */
 
 void LLVMInitializeCore(LLVMPassRegistryRef R);
 
 /** Deallocate and destroy all ManagedStatic variables.
     @see llvm::llvm_shutdown
     @see ManagedStatic */

  * See http://llvm.org/docs/LangRef.html#parameter-attributes
  * and http://llvm.org/docs/LangRef.html#function-attributes
  * for the list of available attributes.
  *
  * NB: Attribute names and/or id are subject to change without
  * going through the C API deprecation cycle.
  */
 unsigned LLVMGetAttributeKindForName(const char *Name, size_t SLen);
+unsigned LLVMGetLastAttrKind();
+
+/**
+ * Create an attribute without argument.
+ */
+LLVMAttributeRef LLVMCreateAttribute(LLVMContextRef C, unsigned KindID,
+                                     uint64_t Val);
+
+/**
+ * Get the unique id corresponding to the target independent attribute
+ * passed as argument.
+ */
+unsigned LLVMGetAttributeKindAsEnum(LLVMAttributeRef A);
+
+/**
+ * Create a string attribute.
+ */
+LLVMAttributeRef LLVMCreateStringAttribute(LLVMContextRef C,
+                                           const char *N, unsigned NLength,
+                                           const char *V, unsigned VLength);
+
+/**
+ * Get the target dependent attribute name.
+ */
+const char *LLVMGetAttributeKindAsString(LLVMAttributeRef A, unsigned *Length);
+
+/**
+ * Check for the different kind of attributes.
+ */
+LLVMBool LLVMIsEnumAttribute(LLVMAttributeRef A);
+LLVMBool LLVMIsIntAttribute(LLVMAttributeRef A);
+LLVMBool LLVMIsStringAttribute(LLVMAttributeRef A);
 
 /**
  * @}
  */
 
 /**
  * @defgroup LLVMCCoreModule Modules
  *

  */
 void LLVMSetGC(LLVMValueRef Fn, const char *Name);
 
 /**
  * Add an attribute to a function.
  *
  * @see llvm::Function::addAttribute()
  */
 void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA);
 
+void LLVMAddAttributeAtIndex(LLVMValueRef F, LLVMAttributeIndex Idx,
+                             LLVMAttributeRef A);
+LLVMAttributeRef LLVMGetAttributeAtIndex(LLVMValueRef F,
+                                         LLVMAttributeIndex Idx,
+                                         unsigned KindID);
+void LLVMRemoveAttributeAtIndex(LLVMValueRef F, LLVMAttributeIndex Idx,
+                                unsigned KindID);
+
 /**
  * Add a target-dependent attribute to a function
  * @see llvm::AttrBuilder::addAttribute()
  */
 void LLVMAddTargetDependentFunctionAttr(LLVMValueRef Fn, const char *A,
                                         const char *V);
 
 /**

  */
 LLVMValueRef LLVMGetPreviousParam(LLVMValueRef Arg);
 
 /**
  * Add an attribute to a function argument.
  *
  * @see llvm::Argument::addAttr()
  */
 void LLVMAddAttribute(LLVMValueRef Arg, LLVMAttribute PA);
 
 /**
  * Remove an attribute from a function argument.
  *
  * @see llvm::Argument::removeAttr()
  */
 void LLVMRemoveAttribute(LLVMValueRef Arg, LLVMAttribute PA);
 
 /**
  * Get an attribute from a function argument.
  */
 LLVMAttribute LLVMGetAttribute(LLVMValueRef Arg);
 
 /**
  * Set the alignment for a function parameter.

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

include/llvm-c/Types.h

 
 /**
  * Used to get the users and usees of a Value.
  *
  * @see llvm::Use */
 typedef struct LLVMOpaqueUse *LLVMUseRef;
 
 /**
+ * Used to represent an attributes.
+ *
+ * @see llvm::Attribute
+ */
+typedef struct LLVMOpaqueAttributeRef *LLVMAttributeRef;
+
+/**
  * @see llvm::DiagnosticInfo
  */
 typedef struct LLVMOpaqueDiagnosticInfo *LLVMDiagnosticInfoRef;
 
 /**
  * @}
  */
 
 #ifdef __cplusplus
 }
 #endif
 
 #endif

include/llvm/IR/Attributes.h

 #ifndef LLVM_IR_ATTRIBUTES_H
 #define LLVM_IR_ATTRIBUTES_H
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/FoldingSet.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/PointerLikeTypeTraits.h"
+#include "llvm-c/Types.h"
 #include <bitset>
 #include <cassert>
 #include <map>
 #include <string>
 
 namespace llvm {
 
 class AttrBuilder;
public:
   bool operator!=(Attribute A) const { return pImpl != A.pImpl; }
 
   /// \brief Less-than operator. Useful for sorting the attributes list.
   bool operator<(Attribute A) const;
 
   void Profile(FoldingSetNodeID &ID) const {
     ID.AddPointer(pImpl);
   }
+
+  /// \brief Return a raw pointer that uniquely identifies this attribute.
+  void *getRawPointer() const {
+    return pImpl;
+  }
+
+  /// \brief Get an attribute from a raw pointer created by getRawPointer.
+  static Attribute fromRawPointer(void *RawPtr) {
+    return Attribute(reinterpret_cast<AttributeImpl*>(RawPtr));
+  }
 };
 
+// Specialized opaque value conversions.
+inline LLVMAttributeRef wrap(Attribute Attr) {
+  return reinterpret_cast<LLVMAttributeRef>(Attr.getRawPointer());
+}
+
+// Specialized opaque value conversions.
+inline Attribute unwrap(LLVMAttributeRef Attr) {
+  return Attribute::fromRawPointer(Attr);
+}
+
 //===----------------------------------------------------------------------===//
 /// \class
 /// \brief This class holds the attributes for a function, its return value, and
 /// its parameters. You access the attributes for each of them via an index into
 /// the AttributeSet object. The function attributes are at index
 /// `AttributeSet::FunctionIndex', the return value is at index
 /// `AttributeSet::ReturnIndex', and the attributes for the parameters start at
 /// index `1'.

include/llvm/IR/Function.h

bool hasFnAttribute(Attribute::AttrKind Kind) const {
     return AttributeSets.hasFnAttribute(Kind);
   }
   bool hasFnAttribute(StringRef Kind) const {
     return AttributeSets.hasAttribute(AttributeSet::FunctionIndex, Kind);
   }
 
   /// @brief Return the attribute for the given attribute kind.
   Attribute getFnAttribute(Attribute::AttrKind Kind) const {
-    if (!hasFnAttribute(Kind))
-      return Attribute();
-    return AttributeSets.getAttribute(AttributeSet::FunctionIndex, Kind);
+    return getAttribute(AttributeSet::FunctionIndex, Kind);
   }
   Attribute getFnAttribute(StringRef Kind) const {
-    return AttributeSets.getAttribute(AttributeSet::FunctionIndex, Kind);
+    return getAttribute(AttributeSet::FunctionIndex, Kind);
   }
 
   /// \brief Return the stack alignment for the function.
   unsigned getFnStackAlignment() const {
     if (!hasFnAttribute(Attribute::StackAlignment))
       return 0;
     return AttributeSets.getStackAlignment(AttributeSet::FunctionIndex);
   }
 
   /// hasGC/getGC/setGC/clearGC - The name of the garbage collection algorithm
   ///                             to use during code generation.
   bool hasGC() const {
     return getSubclassDataFromValue() & (1<<14);
   }
   const std::string &getGC() const;
   void setGC(std::string Str);
   void clearGC();
 
   /// @brief adds the attribute to the list of attributes.
   void addAttribute(unsigned i, Attribute::AttrKind attr);
 
+  /// @brief adds the attribute to the list of attributes.
+  void addAttribute(unsigned i, Attribute Attr);
+
   /// @brief adds the attributes to the list of attributes.
   void addAttributes(unsigned i, AttributeSet attrs);
 
   /// @brief removes the attribute from the list of attributes.
   void removeAttribute(unsigned i, Attribute::AttrKind attr);
 
   /// @brief removes the attributes from the list of attributes.
   void removeAttributes(unsigned i, AttributeSet attr);
 
   /// @brief check if an attributes is in the list of attributes.
   bool hasAttribute(unsigned i, Attribute::AttrKind attr) const {
     return getAttributes().hasAttribute(i, attr);
   }
 
+  Attribute getAttribute(unsigned i, Attribute::AttrKind Kind) const {
+    return AttributeSets.getAttribute(i, Kind);
+  }
+
+  Attribute getAttribute(unsigned i, StringRef Kind) const {
+    return AttributeSets.getAttribute(i, Kind);
+  }
+
   /// @brief adds the dereferenceable attribute to the list of attributes.
   void addDereferenceableAttr(unsigned i, uint64_t Bytes);
 
   /// @brief adds the dereferenceable_or_null attribute to the list of
   /// attributes.
   void addDereferenceableOrNullAttr(unsigned i, uint64_t Bytes);
 
   /// @brief Extract the alignment for a call or parameter (0=unknown).

lib/IR/AttributeImpl.h

class AttributeSetNode final
   // AttributesSetNode is uniqued, these should not be publicly available.
   void operator=(const AttributeSetNode &) = delete;
   AttributeSetNode(const AttributeSetNode &) = delete;
 public:
   void operator delete(void *p) { ::operator delete(p); }
 
   static AttributeSetNode *get(LLVMContext &C, ArrayRef<Attribute> Attrs);
 
+  /// \brief Return the number of attributes this AttributeSet contains.
+  unsigned getNumAttributes() const { return NumAttrs; }
+
   bool hasAttribute(Attribute::AttrKind Kind) const {
     return AvailableAttrs & ((uint64_t)1) << Kind;
   }
   bool hasAttribute(StringRef Kind) const;
   bool hasAttributes() const { return NumAttrs != 0; }
 
   Attribute getAttribute(Attribute::AttrKind Kind) const;
   Attribute getAttribute(StringRef Kind) const;

lib/IR/Core.cpp

 
 #define GET_ATTR_KIND_FROM_NAME
 #include "AttributesCompatFunc.inc"
 
 unsigned LLVMGetAttributeKindForName(const char *Name, size_t SLen) {
   return getAttrKindFromName(StringRef(Name, SLen));
 }
 
+unsigned LLVMGetLastAttrKind() {
+  return Attribute::AttrKind::EndAttrKinds;
+}
+
+LLVMAttributeRef LLVMCreateAttribute(LLVMContextRef C, unsigned KindID,
+                                     uint64_t Val) {
+  return wrap(Attribute::get(*unwrap(C), (Attribute::AttrKind)KindID, Val));
+}
+
+unsigned LLVMGetAttributeKindAsEnum(LLVMAttributeRef A) {
+  return unwrap(A).getKindAsEnum();
+}
+
+LLVMAttributeRef LLVMCreateStringAttribute(LLVMContextRef C,
+                                           const char *N, unsigned NLength,
+                                           const char *V, unsigned VLength) {
+  return wrap(Attribute::get(*unwrap(C), StringRef(N, NLength),
+                             StringRef(V, VLength)));
+}
+
+const char *LLVMGetAttributeKindAsString(LLVMAttributeRef A,
+                                         unsigned *Length) {
+  auto S = unwrap(A).getKindAsString();
+  *Length = S.size();
+  return S.data();
+}
+
+LLVMBool LLVMIsEnumAttribute(LLVMAttributeRef A) {
+  return unwrap(A).isEnumAttribute();
+}
+
+LLVMBool LLVMIsIntAttribute(LLVMAttributeRef A) {
+  return unwrap(A).isIntAttribute();
+}
+
+LLVMBool LLVMIsStringAttribute(LLVMAttributeRef A) {
+  return unwrap(A).isStringAttribute();
+}
+
 char *LLVMGetDiagInfoDescription(LLVMDiagnosticInfoRef DI) {
   std::string MsgStorage;
   raw_string_ostream Stream(MsgStorage);
   DiagnosticPrinterRawOStream DP(Stream);
 
   unwrap(DI)->print(DP);
   Stream.flush();
 
void LLVMAddFunctionAttr(LLVMValueRef Fn, LLVMAttribute PA) {
   AttrBuilder B(PA);
   const AttributeSet PALnew =
     PAL.addAttributes(Func->getContext(), AttributeSet::FunctionIndex,
                       AttributeSet::get(Func->getContext(),
                                         AttributeSet::FunctionIndex, B));
   Func->setAttributes(PALnew);
 }
 
+void LLVMAddAttributeAtIndex(LLVMValueRef F, LLVMAttributeIndex Idx,
+                             LLVMAttributeRef A) {
+  unwrap<Function>(F)->addAttribute(Idx, unwrap(A));
+}
+
+LLVMAttributeRef LLVMGetAttributeAtIndex(LLVMValueRef F,
+                                         LLVMAttributeIndex Idx,
+                                         unsigned KindID) {
+  return wrap(unwrap<Function>(F)->getAttribute(Idx,
+                                                (Attribute::AttrKind)KindID));
+}
+
+void LLVMRemoveAttributeAtIndex(LLVMValueRef F, LLVMAttributeIndex Idx,
+                                unsigned KindID) {
+  unwrap<Function>(F)->removeAttribute(Idx, (Attribute::AttrKind)KindID);
+}
+
 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);

lib/IR/Function.cpp

 }
 
 void Function::addAttribute(unsigned i, Attribute::AttrKind attr) {
   AttributeSet PAL = getAttributes();
   PAL = PAL.addAttribute(getContext(), i, attr);
   setAttributes(PAL);
 }
 
+void Function::addAttribute(unsigned i, Attribute Attr) {
+  AttributeSet PAL = getAttributes();
+  PAL = PAL.addAttribute(getContext(), i, Attr);
+  setAttributes(PAL);
+}
+
 void Function::addAttributes(unsigned i, AttributeSet attrs) {
   AttributeSet PAL = getAttributes();
   PAL = PAL.addAttributes(getContext(), i, attrs);
   setAttributes(PAL);
 }
 
 void Function::removeAttribute(unsigned i, Attribute::AttrKind attr) {
   AttributeSet PAL = getAttributes();

test/Bindings/llvm-c/invoke.ll

unwind5:                                          ; preds = %unwind3
   br i1 %11, label %catch, label %unwind7
 
 unwind7:                                          ; preds = %unwind5
   resume { i8*, i32 } %4
 }
 
 declare void @_D6object6Object6__ctorFMC6object6ObjectZv(%C6object6Object*)
 
-declare i8* @_d_allocmemory(i64)
+declare noalias i8* @_d_allocmemory(i64)
 
 declare i32 @__sd_eh_personality(i32, i32, i64, i8*, i8*)
 
-declare void @__sd_eh_throw(%C6object9Throwable*)
+declare void @__sd_eh_throw(%C6object9Throwable* nonnull) #0
 
 ; Function Attrs: nounwind readnone
-declare i32 @llvm.eh.typeid.for(i8*) #0
+declare i32 @llvm.eh.typeid.for(i8*) #1
 
-attributes #0 = { nounwind readnone }
+attributes #0 = { noreturn }
+attributes #1 = { nounwind readnone }
 No newline at end of file

tools/llvm-c-test/echo.cpp

FunDecl:
   Begin = LLVMGetFirstFunction(Src);
   End = LLVMGetLastFunction(Src);
   if (!Begin) {
     if (End != nullptr)
       report_fatal_error("Range has an end but no begining");
     return;
   }
 
+  auto Ctx = LLVMGetModuleContext(M);
+
   Cur = Begin;
   Next = nullptr;
   while (true) {
     const char *Name = LLVMGetValueName(Cur);
     if (LLVMGetNamedFunction(M, Name))
       report_fatal_error("Function already cloned");
-    LLVMAddFunction(M, Name, LLVMGetElementType(TypeCloner(M).Clone(Cur)));
+    auto Ty = LLVMGetElementType(TypeCloner(M).Clone(Cur));
+    auto F = LLVMAddFunction(M, Name, Ty);
+
+    // Copy attributes
+    for (int i = LLVMAttributeFunctionIndex, c = LLVMCountParams(F);
+         i <= c; ++i) {
+      for (unsigned k = 0, e = LLVMGetLastAttrKind(); k < e; ++k) {
+        if (LLVMGetAttributeAtIndex(Cur, i, k)) {
+          auto A = LLVMCreateAttribute(Ctx, k, 0);
+          LLVMAddAttributeAtIndex(F, i, A);
+        }
+      }
+    }
 
     Next = LLVMGetNextFunction(Cur);
     if (Next == nullptr) {
       if (Cur != End)
         report_fatal_error("Last function does not match End");
       break;
     }