This is an archive of the discontinued LLVM Phabricator instance.

Map Attribute in the C API.
ClosedPublic

Authored by deadalnix on Apr 15 2016, 2:58 PM.

Details

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.

Diff Detail

Event Timeline

deadalnix updated this revision to Diff 53955.Apr 15 2016, 2:58 PM
deadalnix retitled this revision from to Make sure we have a Add/Remove/Has function for various thing that can have attribute..
deadalnix updated this object.
deadalnix added a subscriber: llvm-commits.
Wallbraker requested changes to this revision.EditedApr 19 2016, 4:49 PM
Wallbraker edited edge metadata.

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.

This revision now requires changes to proceed.Apr 19 2016, 4:49 PM

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
2013

LLVMAddFunctionAttr

2128

LLVMAddAttribute

2135

LLVMRemoveAttribute

2591

LLVMAddInstrAttribute

2592

LLVMRemoveInstrAttribute

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 updated this revision to Diff 55139.Apr 26 2016, 5:33 PM
deadalnix edited edge metadata.

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 edited edge metadata.Apr 26 2016, 5:36 PM

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

deadalnix retitled this revision from Make sure we have a Add/Remove/Has function for various thing that can have attribute. to Map Attribute in the C API..Apr 26 2016, 5:36 PM
deadalnix updated this object.Apr 26 2016, 5:36 PM
deadalnix edited edge metadata.
deadalnix updated this revision to Diff 55140.Apr 26 2016, 5:39 PM

Add a way to query attribute kind.

deadalnix updated this revision to Diff 55141.Apr 26 2016, 5:45 PM

Some renaming

whitequark added inline comments.Apr 26 2016, 5:48 PM
include/llvm-c/Core.h
510

Document this function?

deadalnix updated this revision to Diff 55145.Apr 26 2016, 5:50 PM

Add doc for LLVMGetAttributeKind

I have no more objections.

jyknight edited edge metadata.Apr 26 2016, 8:42 PM

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.)

@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 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.

@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 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.

@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.

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.

@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.

@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 added a comment.EditedApr 28 2016, 2:38 AM

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.

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++ .

@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 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.

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'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.

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 updated this revision to Diff 56523.May 8 2016, 2:16 PM
deadalnix edited edge metadata.

Ping

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...)

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.

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);

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);

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?

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.

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.

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.

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.

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 updated this revision to Diff 59668.Jun 5 2016, 9:30 AM

Sample of the current proposal. Also pingpingping !

Wallbraker requested changes to this revision.Jun 8 2016, 3:40 AM
Wallbraker edited edge metadata.

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.

This revision now requires changes to proceed.Jun 8 2016, 3:40 AM

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 updated this revision to Diff 60435.Jun 10 2016, 9:57 PM
deadalnix edited edge metadata.

Add support for string attribute.

deadalnix updated this revision to Diff 60436.Jun 10 2016, 11:39 PM
deadalnix edited edge metadata.

Add echo test

whitequark added inline comments.Jun 11 2016, 5:13 PM
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.

whitequark requested changes to this revision.Jun 11 2016, 5:13 PM
whitequark edited edge metadata.
This revision now requires changes to proceed.Jun 11 2016, 5:13 PM
deadalnix marked an inline comment as done.Jun 11 2016, 5:19 PM
deadalnix added inline comments.
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 added inline comments.Jun 11 2016, 5:26 PM
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 added inline comments.Jun 11 2016, 5:33 PM
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 added inline comments.Jun 11 2016, 5:35 PM
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 added inline comments.Jun 11 2016, 5:40 PM
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 added inline comments.Jun 11 2016, 5:50 PM
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).

As per IRC discussion, some renaming

include/llvm-c/Core.h
505

LLVMGetLastAttributeKind

522

LLVMCreateStringAttribute

536

LLVMIsFlagAttribute?

deadalnix added inline comments.Jun 11 2016, 7:05 PM
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 updated this revision to Diff 60454.Jun 11 2016, 7:12 PM
deadalnix edited edge metadata.

Various name update.

deadalnix updated this revision to Diff 60457.Jun 11 2016, 8:01 PM

LLVMGetLastAttributeKind

deadalnix updated this revision to Diff 60458.Jun 11 2016, 10:31 PM

Various updates as per discussion with @whitequark

whitequark accepted this revision.Jun 11 2016, 10:32 PM
whitequark edited edge metadata.
deadalnix updated this revision to Diff 60459.Jun 11 2016, 10:43 PM
deadalnix edited edge metadata.

Update release notes

This revision was automatically updated to reflect the committed changes.