Page MenuHomePhabricator

[clang] New __attribute__((__clang_arm_mve_alias)).
ClosedPublic

Authored by simon_tatham on Sep 4 2019, 5:47 AM.

Details

Summary

This allows you to declare a function with a name of your choice (say
foo), but have clang treat it as if it were a builtin function (say
__builtin_foo), by writing

static __inline__ __attribute__((__clang_arm_mve_alias(__builtin_foo)))
int foo(args);

I'm intending to use this for the ACLE intrinsics for MVE, which have
to be polymorphic on their argument types and also need to be
implemented by builtins. To avoid having to implement the polymorphism
with several layers of nested _Generic and make error reporting
hideous, I want to make all the user-facing intrinsics correspond
directly to clang builtins, so that after clang resolves
attribute((overloadable)) polymorphism it's already holding the
right BuiltinID for the intrinsic it selected.

However, this commit itself just introduces the new attribute, and
doesn't use it for anything.

To avoid unanticipated side effects if this attribute is used to make
aliases to other builtins, there's a restriction mechanism: only
(BuiltinID, alias) pairs that are approved by the function
ArmMveAliasValid() will be permitted. At present, that function
doesn't permit anything, because the Tablegen that will generate its
list of valid pairs isn't yet implemented. So the only test of this
facility is one that checks that an unapproved builtin _can't_ be
aliased.

Event Timeline

simon_tatham created this revision.Sep 4 2019, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2019, 5:47 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

It somehow doesn't seem entirely good to provide a way to mark arbitrary function in sources as a builtin..
Why can't those MVE builtins be implemented similarly like all the existing builtins?

Sorry about that – I didn't want to put the discussion of rationale in too many different places. The commit message for the followup patch D67161 discusses it a bit.

The usual thing in previous intrinsics systems like NEON is that you have a wrapper function in the header file which calls the builtin. That already doesn't work in every situation, because some intrinsics need one of their arguments to be a compile-time integer constant expression, and with a wrapper function in the way, the constantness property is lost between the user's call site and the builtin. So then you implement those builtins as macros instead of wrapper functions.

The MVE intrinsics spec adds the wrinkle that there are polymorphic functions – with more concise names, overloaded on the argument types. For example, vaddq(v,w) can behave like vaddq_s32 or vaddq_f16 or whatever, depending on what vector types you give it. Those have to be implemented using either __attribute__((overloadable)) or _Generic.

__attribute__((overloadable)) works fine for wrapper functions in the header file, but not for that awkward subset of the functions that have to fall back to being macros. For those, you have to use _Generic (if you're not willing to do what I've done here).

_Generic is immensely awkward, because the difficult thing about it is that all the selection branches not taken still have to type-check successfully. So you can't just say something like 'if this is a uint16x8_t then expand to vfooq_u16, else vfooq_f16, etc', because all but one of those will be type errors. Instead you have to pile in endless workarounds in which each branch of the _Generic is filled with sub-Generics that coerce the arguments in untaken branches to something that is nonsense but won't cause a type-check error – and hope that none of your 'replace it with validly typed nonsense' workarounds won't accidentally trigger in any case where the branch is taken.

Also, if you need to disambiguate based on more than one of the argument types (which can happen in this spec), then you have the same problem again: you can't nest a _Generic switching on argument 2 inside each branch of an outer one switching on argument 1, because as soon as the set of legal type pairs isn't a full Cartesian product, the inner Generic of an untaken branch fails to type-check again, so you need yet more layers of workaround.

And after you've done all that, the error reporting is hideous. It's almost as bad as those C++ error messages gcc used to generate in which it had expanded out some STL type in the user's code into three whole pages of library-internals nonsense.

Doing it this way, what happens is that first clang resolves the __attribute__((overloadable)) based on all the argument types at once; then, once it's decided which function declaration is the interesting one, it looks up the BuiltinId that I put there using this mechanism; and then it's looking at a call directly to that builtin from the user's code, so it can check constant arguments at their original call site. It needs almost no code; no enormous chain of ugly workarounds; and if the user passes an invalid list of argument types, the error report is beautiful: clang will show you each declaration of the polymorphic name from the header file, and legibly explain why each one doesn't match the arguments the user actually passed.

So, I hear you: this is something clang has always found a way to do without before. But if I can't do it this way, could you suggest a way that I could get anything like that really nice error reporting, under all those constraints at once?

I share in the discomfort expressed for this attribute, but I don't have a better solution in mind just yet, so I'm giving some other review feedback in the meantime.

clang/include/clang/Basic/Attr.td
596

Do you expect this attribute to be inherited on redeclarations? I suspect this should be an InheritableAttr.

Also, add a newline above it for visual separation, please.

Finally, should this be a target-specific attribute so that it's only available for your target, or do you expect this attribute to be used on all target architectures?

597

This should not use the GCC spelling as it's not a GCC attribute. It should likely be a Clang spelling instead.

600

No new undocumented attributes, please.

clang/lib/Sema/SemaDeclAttr.cpp
7190

You should be able to call handleSimpleAttribute<ClangBuiltinOverrideAttr>() instead.

simon_tatham marked 2 inline comments as done.Sep 5 2019, 3:38 AM

On the general discomfort with this attribute existing: I'd be happy to lock it down, or mark it as "not recommended" in some way, if that's any help. I don't personally intend any use of it outside a single system header file (namely arm_mve.h, which D67161 will introduce the initial version of).

A warning along the lines of "don't use this!", automatically suppressed by the usual change of warning settings in system headers, would seem like a perfectly reasonable precaution, for example.

clang/include/clang/Basic/Attr.td
596

For my use case, I have no opinion about redeclarations: I expect to declare each affected function exactly once. If you think InheritableAttr is a more sensible default choice, I'm fine with that.

Target-specific: I don't have a use case outside the ARM target, so I'd be happy to lock it down that way if you want.

600

OK. I'd intended to leave it undocumented in order to discourage people from using it in any context other than a system header file. But fair enough – perhaps it should be documented even so.

On the general discomfort with this attribute existing: I'd be happy to lock it down, or mark it as "not recommended" in some way, if that's any help. I don't personally intend any use of it outside a single system header file (namely arm_mve.h, which D67161 will introduce the initial version of).

I think that would at least be something - make it print big fat undisableable warning if used outside of a system header.

A warning along the lines of "don't use this!", automatically suppressed by the usual change of warning settings in system headers, would seem like a perfectly reasonable precaution, for example.

Come to think of it, it would also not be too hard to constrain it to only be usable for a particular subset of builtins, and perhaps even only with a particular set of alias names for them. (I could easily derive all that information from the same Tablegen that arm_mve.h itself is made from.)

Come to think of it, it would also not be too hard to constrain it to only be usable for a particular subset of builtins, and perhaps even only with a particular set of alias names for them. (I could easily derive all that information from the same Tablegen that arm_mve.h itself is made from.)

I think this might be a good idea to explore. In that case, I would recommend naming the attribute __clang_arm_mve_builtin to make it obvious that this attribute has a very specific use in mind.

clang/include/clang/Basic/Attr.td
596

I think it should be an InheritableAttr that is target-specific. We can always expand the targets later if we think the attribute is generally useful.

600

Documenting it still helps developers even if the documentation effectively states that something is for internal use only.

New version which renames the attribute to be MVE-specific, and locks it down as requested.

simon_tatham retitled this revision from [clang] New __attribute__((__clang_builtin)). to [clang] New __attribute__((__clang_arm_mve_alias))..Sep 11 2019, 4:55 AM
simon_tatham edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Sep 17 2019, 6:22 AM
clang/include/clang/Basic/Attr.td
596

Add a newline before the attribute definition for some separation.

clang/include/clang/Basic/AttrDocs.td
4402

will declare -> declares

clang/include/clang/Basic/DiagnosticASTKinds.td
453 ↗(On Diff #219691)

How about: "'__clang_arm_mve_alias' attribute can only be applied to an ARM MVE builtin"

clang/lib/AST/Decl.cpp
3103

Comment should have a FIXME prefix, but tbh, I'm a little uncomfortable adopting the attribute without this being implemented. I assume the plan is to tablegen a header file that gets included here to provide the lookup?

3118–3119
if (const auto *AMAA = getAttr<ArmMveAliasAttr>) {
  IdentifierInfo *II = AMAA->getBuiltinName();
  ...
}
3123

I'm not certain how comfortable I am with having this function produce a diagnostic. That seems like unexpected behavior for a function attempting to get a builtin ID. I think this should be the responsibility of the caller.

clang/test/CodeGen/arm-mve-intrinsics/alias-attr.c
1 ↗(On Diff #219691)

This seems like a Sema test, not a CodeGen test.

There are other Sema tests missing: the attribute only accepts one arg instead of zero or two+, only accepts identifier args (test with a string literal and an integer literal), only applies to functions, etc.

Should there be a diagnostic if the attribute is applied to a function with internal linkage (or are there static builtins)? What about member functions of a class?

Once the builtin hookup is complete, there should also be tests for the attribute being accepted properly, and codegen tests demonstrating the proper builtin is called.

simon_tatham marked 10 inline comments as done.Oct 2 2019, 2:56 AM
simon_tatham added inline comments.
clang/lib/AST/Decl.cpp
3103

Yes: D67161, which I intend to commit as part of the same patch series once everything in it is approved, fills in this function with tablegen output.

I could roll both into the same monolithic patch, but I thought it would be better to break it up into manageable pieces as far as possible, especially when some of them (like this one) would need to be reviewed by people with significantly different expertise from the rest.

3123

The caller? But there are many possible callers of this function. You surely didn't mean to suggest duplicating the diagnostic at all those call sites.

Perhaps it would make more sense to have all the calculation in this getBuiltinID method move into a function called once, early in the FunctionDecl's lifetime, which figures out the builtin ID (if any) and stashes it in a member variable? Then that would issue the diagnostic, if any (and it would be called from a context where that was a sensible thing to do), and getBuiltinID itself would become a mere accessor function.

clang/test/CodeGen/arm-mve-intrinsics/alias-attr.c
1 ↗(On Diff #219691)

Thanks for pointing out the rest of those missing test cases; I'll add them.

The intended use of this attribute is very similar to the example I show here (only with one of the upcoming MVE builtins in place of __builtin_arm_nop): declaring a function as static __inline__ that corresponds to a builtin (either by name, or via this new attribute) has the effect of filling in the prototype of the function at compile time if it wasn't baked into Builtins.def up front. (Which I'll need to do for the MVE builtins, because some of them will have struct types as arguments that won't exist until the header file has defined them.)

I could add a diagnostic if the attribute is applied to a function with external linkage, and/or one for class member functions, if you think either one is important. (I don't expect to need either of those for my intended use case.)

simon_tatham marked an inline comment as done.
simon_tatham edited the summary of this revision. (Show Details)

Addressed many (but not quite all) review comments.

simon_tatham marked an inline comment as done.Oct 2 2019, 3:17 AM
simon_tatham added inline comments.
clang/lib/Sema/SemaDeclAttr.cpp
7190

Oh, nearly forgot: apparently I can't, because that template expects the attribute to have a constructor that takes only an ASTContext and an AttributeCommonInfo. But the constructor for my attribute also takes an IdentifierInfo giving the builtin name.

aaron.ballman marked 2 inline comments as done.Oct 3 2019, 12:07 PM
aaron.ballman added inline comments.
clang/lib/AST/Decl.cpp
3103

Ah, I didn't realize that was so involved; keeping them split makes sense.

3123

The caller? But there are many possible callers of this function. You surely didn't mean to suggest duplicating the diagnostic at all those call sites.

Yes, I did. :-) No caller is going to expect that calling a const function that gets a builtin ID is going to issue diagnostics and so this runs the risk of generating diagnostics in surprising situations, such as from AST matchers.

Perhaps it would make more sense to have all the calculation in this getBuiltinID method move into a function called once, early in the FunctionDecl's lifetime, which figures out the builtin ID (if any) and stashes it in a member variable? Then that would issue the diagnostic, if any (and it would be called from a context where that was a sensible thing to do), and getBuiltinID itself would become a mere accessor function.

That might make sense, but I don't have a good idea of what performance concerns that might raise. If there are a lot of functions and we never need to check if they have a builtin ID, that could be expensive for little gain.

clang/lib/Sema/SemaDeclAttr.cpp
7190

Ah, good point.

simon_tatham marked an inline comment as done.Oct 4 2019, 2:28 AM
simon_tatham added inline comments.
clang/lib/AST/Decl.cpp
3123

OK – so actually what you meant to suggest was to put the diagnostic at just some of the call sites for getBuiltinId?

With the intended behavior being that the Sema test in this patch should still provoke all the expected diagnostics in an ordinary compilation context, but in other situations like AST matchers, it would be better for getBuiltinId to silently returns 0 if there's an illegal ArmMveAlias attribute?

(I'm just checking I've understood you correctly before I do the work...)

aaron.ballman added inline comments.Oct 4 2019, 11:25 AM
clang/lib/AST/Decl.cpp
3123

OK – so actually what you meant to suggest was to put the diagnostic at just some of the call sites for getBuiltinId?

Yes! Sorry, I can see how I was unclear before. :-)

With the intended behavior being that the Sema test in this patch should still provoke all the expected diagnostics in an ordinary compilation context, but in other situations like AST matchers, it would be better for getBuiltinId to silently returns 0 if there's an illegal ArmMveAlias attribute?

Yes. getBuiltinId() already returns 0 in error cases without diagnosing, such as the function being unnamed or not being a builtin. I want to retain that property -- this function returns zero if the function is not a builtin. It's up to the caller of the function to decide whether a zero return value should be diagnosed or not.

To be honest, this diagnostic feels like it belongs in SemaDeclAttr.cpp; it is placing a constraint on which declarations can have the attribute, so that should be checked *before* applying the attribute to the declaration. This also keeps the AST cleaner by not having an attribute on a function which should not be attributed.

simon_tatham marked an inline comment as done.Oct 7 2019, 2:47 AM
simon_tatham added inline comments.
clang/lib/AST/Decl.cpp
3123

getBuiltinId() already returns 0 in error cases without diagnosing

Ah, I hadn't spotted that! That by itself makes it all make a lot more sense to me.

this diagnostic feels like it belongs in SemaDeclAttr.cpp

OK, I'll look at moving it there. Thanks for the pointer.

Moved the diagnostic into SemaDeclAttr.cpp as suggested.

simon_tatham marked an inline comment as done.Oct 7 2019, 6:32 AM
simon_tatham added inline comments.
clang/lib/AST/Decl.cpp
3123

I made this change, and discovered that as a side effect, this diagnostic is now reported on the same one of the two source lines as all the others in my test file – it's now reported against the attribute rather than the end of the declaration it's applied to.

I guess that's extra evidence that you were right about where it belongs :-)

Rebased to current master.

aaron.ballman marked an inline comment as done.Oct 9 2019, 1:54 PM
aaron.ballman added inline comments.
clang/include/clang/Basic/DiagnosticASTKinds.td
539 ↗(On Diff #224030)

Spurious newline?

clang/lib/AST/Decl.cpp
3123

Fantastic to hear!

clang/lib/Sema/SemaDeclAttr.cpp
4840–4841

You don't need to do this, it should be handled automatically for you in handleCommonAttributeFeatures().

4843–4847

I believe this is also automatically checked for you as well.

simon_tatham marked 3 inline comments as done.

Removed spuriously inserted blank line and redundant checkAttributeNumArgs.

This revision is now accepted and ready to land.Oct 13 2019, 12:13 PM

Thanks for the review!

(I expect to leave this uncommitted until I have enough other patches approved to make it actually useful, and then commit them all together.)

(Rebased to current master; no change)

This revision was automatically updated to reflect the committed changes.