[PATCH] D19181: Map Attribute in the C API.

[whitequark via llvm-commits]

whitequark added inline comments.

================
Comment at: include/llvm-c/Core.h:517
@@ +516,3 @@
+ */
+unsigned LLVMGetAttributeKindAsEnum(LLVMAttributeRef A);
+
----------------
What does this do if a target-dependent attribute is passed? This needs to be documented.

================
Comment at: tools/llvm-c-test/echo.cpp:800
@@ +799,3 @@
+      for (unsigned k = 0, e = LLVMGetEndAttrKind(); k < e; ++k) {
+        if (LLVMGetAttributeAtIndex(Cur, i, k)) {
+          auto A = LLVMCreateAttribute(Ctx, k, 0);
----------------
deadalnix wrote:
> whitequark wrote:
> > deadalnix wrote:
> > > whitequark wrote:
> > > > 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.
> > > 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 ?).
> > `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.
> 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.
> 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?


http://reviews.llvm.org/D19181