Differential D19181
Map Attribute in the C API. Authored by deadalnix on Apr 15 2016, 2:58 PM.
Details 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 TimelineComment Actions 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. Comment Actions 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 ?
Comment Actions 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. Comment Actions 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. Comment Actions One thing I would like to have right away is a way to get the kind of an attribute. Looks good otherwise.
Comment Actions 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.) Comment Actions @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. Comment Actions I'm not sure what you mean? I proposed accessors for the different types of attributes that exist now: flag, int, and string. Comment Actions @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. Comment Actions 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. Comment Actions @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. Comment Actions 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:
That leaves, most importantly, some properties of the original proposal:
Comment Actions
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. Comment Actions @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? Comment Actions 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. Comment Actions 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++ . Comment Actions @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). Comment Actions Then this should be really easy to describe....I'm not sure why you keep pushing back on this. Comment Actions 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 ? Comment Actions 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. Comment Actions 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. Comment Actions I'm still hoping for a full response to my previous question, as the Comment Actions 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. Comment Actions 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);Comment Actions 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); Comment Actions Thanks! Now that I can actually see your proposal, I can respond to it. There's some things missing:
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? Comment Actions True. That's not the most useful use case in practice, but it can be added.
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.
Same as for int attributes + They need a new getter.
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. Comment Actions 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. Comment Actions 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. Comment Actions Please explain. The API I proposed does allow access to all three of the kinds of attributes.
The last proposal from me had no AttributeSet. Comment Actions 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 ? Comment Actions 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.
Comment Actions 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.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||