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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

>>>> I also wonder if all these should cause a clang diagnostic, at least under `-Wall`.
>> 
>> What do you mean?
> 
> These are very much a generally-incompatible, very fragile, extensions.
>  They will not work on any other compiler, and will likely not work
>  on a different version of clang. It is not okay not to diagnose such cases.

It totally depends on your definition of "will not work". They might not be recognized and therefore be ignored, correct. They should not be misinterpreted as (1) the name should make them unambiguous and (2) LLVM-IR needs to be backward compatible anyway. Take different version of clang and `__attribute__((noescape))` for example, or take gcc. All the problems apply but it is even far less obvious that it is not a language feature and will not be portable (across compilers). (Given that various commercial compilers are build on top of LLVM/Clang there is an argument for forward compatibility.) I agree on the diagnose issue, we should, as we do, warn for unrecognized attributes. But again, both clang and gcc do that by default.

As a side node, I actually want to provide user with a "macro-portability-layer" that hides the spelling and even allows to remove the attributes for incompatible compilers.  Though, one step at a time.

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

(See above.)

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

>> 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. 
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've also personally run into situations where I've been either trying create new attributes for LLVM or generating test cases for optimizations that require specific LLVM attributes that otherwise don't exist with Clang's existing frontend.
> 
> I don't think this is a motivating use case for our users, though.

Agreed. This can be a bonus but is not sufficient for integration.

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

>> Off the top of my head, there's a couple of ways we could imagine dealing with versioning:
>> 
>> - We could have the equivalent of the LLVM/Clang version included as part of the attribute (e.g. something like `__attribute__((llvm7_arg(a, "nocapture")))`)
> 
> This will not scale well, but we could add it as an argument to the attribute itself.

I do not believe we need to but I do not have the strongest feeling here. Especially with a "macro-layer" this would be easy to do anyway.

>> - We could let nonexistent attributes simply slide into extraneous string attributes. In its current form it will look for an enum attribute if one exists and if not create a string attribute. I don't believe that extraneous string attributes would cause any harm -- but correct me if I'm wrong.
> 
> I'm not okay with this -- nonexistent attributes need to be diagnosed rather than silently ignored.

Agreed.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63845