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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

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.

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.

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

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

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

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

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

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

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63845