[PATCH] D63845: [WIP] Create a clang attribute that lets users specify LLVM attributes

Aaron Ballman via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Sat Jul 6 09:15:44 PDT 2019


aaron.ballman added a comment.

In D63845#1571855 <https://reviews.llvm.org/D63845#1571855>, @jdoerfert wrote:

> I think we should postpone a detailed discussion until we are ready to send an RFC on this. Nevertheless, I inlined some responses below.
>
> In D63845#1570639 <https://reviews.llvm.org/D63845#1570639>, @aaron.ballman wrote:
>
> > In D63845#1566987 <https://reviews.llvm.org/D63845#1566987>, @jdoerfert wrote:
> >
> > > In D63845#1561983 <https://reviews.llvm.org/D63845#1561983>, @lebedev.ri wrote:
> > >
> > > > In D63845#1561793 <https://reviews.llvm.org/D63845#1561793>, @jdoerfert wrote:
> > > >
> > > > > In D63845#1560605 <https://reviews.llvm.org/D63845#1560605>, @aaron.ballman wrote:
> > > > >
> > > > > > In D63845#1559995 <https://reviews.llvm.org/D63845#1559995>, @lebedev.ri wrote:
> > > > > >
> > > > > > > What's the target use-case here? What can't be solved with normal attributes?
> > > > > >
> > > > >
> > > > >
> > > > > With "normal" you mean something like `__attribute__((noescape))`? We don't have them for all attributes, I doubt we want to but I might be wrong.
> > > >
> > > >
> > > > That is precisely the question.
> > > >  What is the motivation for exposing such LLVM-specific low-level unstable implementation detail?
> > >
> > >
> > > I would disagree to the unstable part. Keeping LLVM-IR backward compatibility is always a priority and changing the meaning of an existing LLVM-IR attribute will break various things already.
> > >  Why would you think it is unstable?
> >
> >
> > Because, to date, we've never made blanket any stability guarantees about LLVM attributes from within the context of Clang. Instead, LLVM attributes are introduced into Clang on a case by case basis when they've been determined to be stable enough to warrant exposing. This extra layer also gives us more flexibility in translating frontend attributes into backend attributes.
>
>
> You still assume LLVM attributes are not stable while I tried to argue they are, they have to be already for backward compatibility, at least target agnostic ones.


I think *many* LLVM attributes have stability to them, especially once they've had some time to bake. It's the odd ones that don't fit the norm that have me uncomfortable. However, because the proposal isn't a blanket addition of *all* LLVM attributes automatically, I'm far less concerned. I didn't pick up on that bit from the original proposal.

> Your example below was `thunk` and you mentioned other problems that can arise but I think we should step back for a moment and talk about the use case here because that should limit the potential problems. For me the use case is as follows: The user, or a tool, wants to encode additional information in the source to aide optimization. The basically only way to do this in IR is to add attributes to one of three locations, (1) function level, (2) return value level, or (3) argument level. For all three we have examples in Clang so we know how do interact with templates, and other potential hurdles. Now it is not the goal to allow all attributes as some of them encode not information but implementation details, e.g., `thunk`. I'm basically interested in attributes that can be safely derived and dropped in he IR today.

That seems like a reasonable decision, to me. I picked on `thunk` only because it was a good example of an attribute that would require additional semantic checks that couldn't be easily automated, but I'm sure there's a subset of LLVM attributes that have easier semantics to deal with. Thank you for the extra clarification.

>>> Also, we basically do expose the low-level parts one by one through "normal" attributes as soon as someone has enough motivation to write all the necessary boilerplate (see more below). Why not merge all the logic/code-gen into a single uniform framework that deals with LLVM-IR attributes?
>> 
>> Because that increases the chance of bad user experiences by lowering the bar that ensures newly added attributes behave well for user code. That boilerplate exists for a reason and forces a developer to make decisions (like what number and kind of arguments are accepted, what spellings make sense for the attribute, what subjects the attribute appertains to, etc).
> 
> We can still make all these decisions, this patch almost makes all of them explicit. We can whitelist arguments (through the switch that exists), we can rename them between the FE and IR if we want to (though I would prefer not to), and we do restrict the positions already to the three interesting ones.

Fantastic! I think that if you take the tablegen approach I suggested, we can hopefully (easily) cover all the random bits of automated checking we've added for frontend attribute checking.

>>> In D63845#1564289 <https://reviews.llvm.org/D63845#1564289>, @aaron.ballman wrote:
>>> 
>>>> > In D63845#1561983 <https://reviews.llvm.org/D63845#1561983>, @lebedev.ri wrote:
>>>> > 
>>>> >> In D63845#1561793 <https://reviews.llvm.org/D63845#1561793>, @jdoerfert wrote:
>>>> >>
>>>> >> > In D63845#1560605 <https://reviews.llvm.org/D63845#1560605>, @aaron.ballman wrote:
>>>> >> >
>>>> >> > > In D63845#1559995 <https://reviews.llvm.org/D63845#1559995>, @lebedev.ri wrote:
>>>> >> > >
>>>> >> > > > What's the target use-case here? What can't be solved with normal attributes?
>>>> >> > >
>>>> >> >
>>>> >> >
>>>> >> > With "normal" you mean something like `__attribute__((noescape))`? We don't have them for all attributes, I doubt we want to but I might be wrong.
>>>> >>
>>>> >>
>>>> >> That is precisely the question.
>>>> >>  What is the motivation for exposing such LLVM-specific low-level unstable implementation detail?
>>>> > 
>>>> > 
>>>> > There's a couple of potential use cases for this -- most of which are more for LLVM developers than end users.
>>>>
>>>> That's why I am hesitant to provide it as an end-user feature. I have not seen a compelling case for why a generic set of user-facing attributes is a good design as opposed to thinking about individual attributes LLVM exposes and carefully thinking how we want to allow users access to it (if at all), which is the status quo. Once we expose this sort of feature to users, they *will* use it even if it isn't meant for them, and you can't unring a bell. What stability guarantees do these attributes have?  How are you planning to handle semantic checking of the attributes (so that attributes which only appertain to some constructs are properly diagnosed when written on the wrong construct)? How do you plan to handle attributes that require arguments (as well as diagnosing improper argument types, etc)? How do you diagnose mutually exclusive backend attributes? These sort of questions are ones that all get answered when exposing individual attributes as needed, but they're problems that need to be solved in a generic way for these three attributes. Also, you're not exposing any statement attributes through this -- does the LLVM backend not have statement-level attributes? What about type attributes?
>>> 
>>> 
>>> These are **a lot of very good questions we have to answer** at some point. I hope we can do so in a constructive and goal oriented fashion instead of using them as a excuse to block advancing the status quo.
>> 
>> My intentions are not to block advancement of the status quo, but when given a patch with a concrete implementation that lacks much of the polish required for a production-quality feature, it's reasonable to push back strongly. This isn't a small feature, so the bar for inclusion is higher.
> 
> Maybe my wording was off but I found it "problematic" that there was harsh critique on a first draft clearly marked as WIP not looking for actual code review (no llvm-commits subscriber).  Maybe this would have needed more background. The patch is part of a GSoC project to explore options and it is clearly not ready for prime time yet. I asked for it to be put here so I can take a look and comment in a familiar environment. Also other peoples feedback is obviously welcomed. The tablegen idea and other things mentioned are very interesting.

Oh holy wow! I did not notice that this hadn't gone out to llvm-commits or cfe-commits for review -- I thought this was a typical patch, which is why I pushed back as strongly as I did (despite the WIP in the title, even). I'm sorry for not noticing these details! I had the impression that this specific design was more concrete than it probably is.

>>> Let me try to give partial answers right away in an effort to show there is a way forward (in addition to the scientific project we will use them for):
>>> 
>>> Use-cases:
>>> 
>>>   There are three, albeit similar, use cases that come to mind:
>>>   1) A performance-aware user interprets analysis remarks (or debug messages) and provides the information that was missing for the optimization they had in mind to fire. This is not an artificial use case, we have that with performance-aware people here (in the labs) and we see emails on the list that ask for help to enable transformations.
>>>   2) We have tooling support that helps less involved/advanced users to determine performance improving opportunities (see https://link.springer.com/chapter/10.1007/978-3-030-20656-7_13) and we need to provide them with a way to actually make use of them.
>>>   3) We want to distribute a library in non-lto mode but without introducing optimization barriers at each call to it. This is the project @wsmoses is working right now (see https://www.llvm.org/OpenProjects.html#header-generation).
>>>    
>>> 
>>> Stability: I argued above already that LLVM-IR attributes are, or better have to be, backward compatible in practice. You would already break systems if they were not.
>>> 
>>> Semantic checking, usage, arguments: For the prototype we have the IR verifier to do this job. Before we (try to) upstream any of this, we need to expose the checking capabilities and use them in the sema. This has to happen. Though, I do not see it as a major obstacle. First, we can whitelist attributes that are allowed in the new scheme (the big switch). Second, (generic) IR attributes are fairly simple, if they have arguments it is a single integer. Lastly, the number of locations IR attributes are placed is fairly limited, the interesting ones are: (a) function/call site attributes, (b) argument/parameter attributes, (c) return value/call site value attributes.
>> 
>> Having done a lot of work to generalize our attribute tablegen to automate semantic checking like this, I see this as more of an obstacle than you do. I suspect you will find that only about 80% of attributes fit the easy cases you envision because there will be attributes that need special requirements (late parsing, template instantiation behaviors, target-specific attributes, etc) and that will be Very Hard to get right. User experience is the biggest concern I have with this feature.
> 
> I really think that >95% of all LLVM-IR optimization attributes are easy. There are certainly other attributes but most of them come with all the problems you described and therefore do not fit a general scheme.
> 
>>>>> The primary driver behind this is a research/GSOC project to try propagating information learned in LLVM analysis/optimization passes back into augmented headers.
>>>> 
>>>> To what end? If we inferred this information successfully, do we gain something by explicitly putting it into a header? One way of exposing these attributes that would make me marginally more comfortable would be to always diagnose use of these attributes as being experimental, with no warning flag group to allow the warning to be disabled. However, if you plan to write these attributes into headers and then distribute those headers for production use, that approach won't work.
>>> 
>>> Let's try it out for science sake, without trying to commit it.
>> 
>> Trying it out in a private branch is a great idea. :-)
> 
> We will before an RFC and a patch for review is produced.

Sounds great!

>>> Also, as mention above already, all the problems mentioned so far, portability/compatibility, stability, etc. exist already for the hand picked attributes we have.
>>>  What do you think of a "macro-portability-layer"?
>> 
>> I think it's an interesting idea, but the important point is that we (to date) *hand pick* attributes from LLVM and bring them into Clang. This is an automation of that process, and that automation is what I'm pushing back against without more information about how we plan to expose these attributes responsibly. That means considering things like "should this attribute be exposed in <language>?", "can this conflict with other attributes", "should this attribute only apply to static global variables, or are thread local variables also of interest", appropriate documentation, etc.
>> 
>> Basically: LLVM attributes are designed with backend requirements in mind, and backend requirements are not generally as user-facing as frontend attributes. What safeguards can be put in place to ensure that these frontend requirements are forced to be considered before exposing a new backend attribute?
> 
> That is not generally true.

I disagree; people who typically review frontend attributes are not the same people who review backend attributes. I have definitely had frontend reviews exposing backend attributes where the author has not thought about all of the messier aspects of the frontend languages the attribute is exposed in.

> There are LLVM "backend attributes", or better "implementation attributes", and LLVM "information attributes". Only the the latter is targeted here and that is where we are constantly adding more of. Information attributes are target agnostic and describe properties of the IR (values). The safeguard is the whitelist mentioned before.

Ah, I didn't know you made a distinction between the two in the backend, that's good to know. Target agnostic attributes are easier than ones with target-specific semantics, but the kinds of design considerations I'm worried about are target-agnostic things like "should this apply to ObjC method decls?" and "what happens when this is written on a virtual function -- how do overriders behave?". The keeplist doesn't safeguard those sort of questions as strongly as a full attribute implementation does, but I think we can find a happy medium that isn't too onerous for anyone.

>>>>>>>>> How is versioning expected to be handled? New attribute vs old clang, and vice versa.
>>>>>>> 
>>>>>>> Unknown attributes are not necessarily a problem, they should compile fine ignoring the attribute, or am I wrong?
>>>>>> 
>>>>>> I don't know, that's why i'm asking.
>>>> 
>>>> The issue I see is that it will be temptingly easy to ignore the attributes silently rather than ignoring the attributes by telling the user they're being ignored and why. The other issue I see is that we (to my knowledge) have no stability guarantees with LLVM attributes that are exposed this way, and this raises the implementation burden for backend attributes.
>>> 
>>> I mentioned warnings, whitelisting, and existing stability guarantees already. Does that lower your wariness?
>> 
>> Yes, but you only mentioned that they needed to be handled -- I'd like to know how with more concrete details. My big worry is that we trivially expose backend attributes that are arcane, undocumented (for frontend use), or provide a bad user experience. As a more concrete example of what I'm afraid of -- we have an LLVM function attribute named "thunk". How does Clang do the semantic checking for that attribute -- it needs to diagnose the attribute when it's ignored, and so if the function never makes a tail call, what diagnostics does the user get? How do you rewrite that documentation so that it makes sense to a frontend user?
> 
> Why would be whitelist an arcane, undocumented attribute? I think you paint the picture to negative here. (I commented on `thunk` and similar attributes above.)

Backend attributes can be arcane by their nature; `thunk` is a great example of that. We have backend documentation for that attribute, but it is not super useful to a Clang user ("The caller is expected to cast the thunk prototype to match the thunk target prototype." -- what does that *mean* to someone adding this attribute to a C function declaration?). What I meant by undocumented is two-fold: 1) it is important that we have *frontend* docs for these attributes that explain the attribute behavior (it doesn't have to go directly into AttrDocs.td, but should be published to the same online resource as AttrDocs.td is), and 2) the frontend docs should not be a copy/paste of the backend docs (or a simple link to them) unless the backend docs are already sufficiently clear to a frontend user, including things like code examples and how the attribute behaves in whatever languages its exposed in. Additionally, we have no test coverage demonstrating proper and improper use of backend attributes like `thunk` in the frontend, which doesn't mean the attribute is automatically arcane, but lacking this sort of testing makes the attribute somewhat more mysterious than it needs to be for someone unfamiliar with the backend attribute semantics. FWIW, this is a lesson we learned with some of our attributes in the frontend; they're totally undocumented and have almost zero test coverage so when questions arise about the attributes, it becomes an unpleasant code archaeology expedition.

>>>>> - We could separate string attributes from enum attributes and raise errors if an enum attribute doesn't exist. This doesn't cover semantic changes, but provides more information than creating it as an extra string attribute (at cost of possible compiler error for the user).
>>>> 
>>>> This is a good first step, but it doesn't solve a lot of the issues I raised above regarding other diagnosable attribute scenarios.
>>> 
>>> We have warnings for unknown "C-level" attributes, e.g., `warning: unknown attribute 'noescapedsa' ignored [-Wunknown-attributes]`, and we have to issue these warnings for unknown LLVM-IR attributes as well.
>> 
>> Unknown attributes are not the only reason an attribute is ignored, though. I am more worried about *known* attributes that are ignored because of code issues, like my example with "thunk" above. I think it should be easy to automate generic checking for things like number of arguments and ignoring target-specific attributes on unsupported targets. I think it will be a little bit harder to ensure attributes appertain to the proper subject and have the correct argument types, but it shouldn't be too bad. I think the other semantic checks that lead to attributes being ignored will be the intractable problem.
> 
> I want "information attributes" describing "properties", such as: `dereferenceable(_globally)`, `align`, `nonull`, `noreturn`, `returned`, `read/write-only`, `readnone`, `noescape`, `inaccessiblememonly_XXX`, `nounwind`, ...
>  Some of these have already a counterpart in Clang, others do not. These do come with semantic "implications" but I don't think the semantic checks we need are particularly hard. We need to make sure they are used at a suitable position (out of the three), and the type of the underlying value is acceptable, e.g., a pointer type for `align`. If you want to check if they actually hold, or trivially not hold, that is a different question which I would not try to answer right now.

Thank you for this list, as it helps me to understand the scope of what you're after. I do agree that many of the semantic checks for these should be easy, but those are not the checks I worry about. Let me pick on `noreturn` for a moment (I know this is already exposed in Clang, but it's an easy example of what I'm thinking about). The semantic checks I worry about there are: what happens if I write noreturn on a virtual function (do all overrides have to be noreturn, or can they vary? if they vary, what happens when calling through a base or derived pointer?)? Can I write noreturn on a function that has a non-void return type? Should we diagnose a `return` statement in such a function? What about a `throw` statement? What happens when I bind such a function to a function pointer -- does the function pointer have the same semantics? (And so on.)

I think it's important that we be asking, answering, documenting, and testing these sort of things on a per-attribute basis as part of this feature. I don't think this is something we will be able to automate from the backend, so I'm hoping we can find a way to have just enough process in place that these questions get asked but without so much process that it becomes an annoying slog for anyone involved.

>> I'm wondering if we can leverage tablegen to solve these concerns. <speculation>What if we used tablegen (from the backend) to generate the skeleton concrete frontend attribute implementation in some sort of automated fashion? So, instead of adding an attribute to a keeplist and using a generic attribute syntax (as in this patch), we exposed the backend attributes like we would any other frontend attribute (using a tablegen member to determine which backend attributes are exposed)? I'm thinking of something like a Clang build target that reaches back into LLVM for its attribute tablegen file and generates an LLVMBackendAttrs.td file for the frontend to consume (that gets included from Attr.td and AttrDocs.td). It could additionally make a SemaDeclLLVMBackendAttrs.inc file to be included into SemaDeclAttr.cpp for declaration attribute processing. This won't cover all of the cases I'm worried about (such as more complex semantic checking), but it gives an easy path to exposing backend attributes to the frontend in a way that's consistent with our other attributes.</speculation> Something where we generated "real" attributes for these would also cut down on the chances of us having code like `if (const auto *A = SomeDecl->getAttr<LLVMFNAttr>()) { if (A->getBackendAttrKind() == "whatever") { ... } }` in the frontend.
> 
> I like it. That would be a nice solution to generate the checking and annotation code in a generic manner.

Other questions I have that may be worth considering early: 1) what spellings do we want to support for these? e.g., should they only be exposed via `[[]]` or is there a reason we should also support `__attribute__(())` spellings? 2) should these go under the `clang` attribute namespace? What sort of name collisions do we have to worry about if it does? Or should they be exposed under an `llvm` (or some other) namespace?

My suggested answers are: 1) only expose as `[[]]` spellings to avoid accidentally conflicting with GNU attributes that share the same name with different semantics, 2) I don't know if we already have naming collisions between LLVM attributes and clang attributes, but if we do (or are worried about them in the future), I think having an `llvm` namespace for these attributes would make the most sense. Also, I can imagine other vendors like GCC wanting to support attributes in the `clang` namespace (like we do with `gnu` attributes), but I don't imagine anyone wanting to support these LLVM attributes the same way, so the logical division may make sense.

I'm guessing there are other questions that will come up as we get into it.

Btw, thank you for working on this -- while I have been critical of the patch due to my own misunderstandings, I do see the utility in this general idea.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63845/new/

https://reviews.llvm.org/D63845





More information about the cfe-commits mailing list