Page MenuHomePhabricator

[clang] New __attribute__((__clang_arm_mve_alias)).
Needs ReviewPublic

Authored by simon_tatham on Wed, Sep 4, 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.Wed, Sep 4, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Sep 4, 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
7442

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

simon_tatham marked 2 inline comments as done.Thu, Sep 5, 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))..Wed, Sep 11, 4:55 AM
simon_tatham edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Tue, Sep 17, 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
4350 ↗(On Diff #219691)

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
3065

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?

3080–3081
if (const auto *AMAA = getAttr<ArmMveAliasAttr>) {
  IdentifierInfo *II = AMAA->getBuiltinName();
  ...
}
3085

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.