Page MenuHomePhabricator

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

Authored by simon_tatham on Jan 10 2020, 9:16 AM.

Details

Summary

This is applied to the vector types defined in <arm_mve.h> for use
with the intrinsics for the ARM MVE vector architecture.

Its purpose is to inhibit lax vector conversions, but only in the
context of overload resolution of the MVE polymorphic intrinsic
functions. This solves an ambiguity problem with polymorphic MVE
intrinsics that take a vector and a scalar argument: the scalar
argument can often have the wrong integer type due to default integer
promotions or unsuffixed literals, and therefore, the type of the
vector argument should be considered trustworthy when resolving MVE
polymorphism.

As part of the same change, I've added the new attribute to the
declarations generated by the MveEmitter Tablegen backend (and
corrected a namespace issue with the other attribute while I was
there).

Diff Detail

Event Timeline

simon_tatham created this revision.Jan 10 2020, 9:16 AM
aaron.ballman added inline comments.Jan 14 2020, 5:52 AM
clang/include/clang/Basic/Attr.td
1482

Should this be a target-specific attribute?

1483

Why does this have a __clang prefix? That seems a bit strange given that the attribute can be spelled [[clang::__clang_arm_mve_strict_polymorphism]]. I'd probably drop the __clang_ prefix entirely.

clang/include/clang/Basic/AttrDocs.td
4805

I think adding a code example of correct vs erroneous code would be useful.

4808

If you have an easy code example demonstrating this, that would be useful as well.

clang/lib/Sema/SemaType.cpp
7566

So this attribute can be written on any type? I would have expected it to be limited to just types that can interact through vector operations. Does it make sense to declare, for instance, a struct having this type?

simon_tatham marked 7 inline comments as done.
  • Renamed the attribute as suggested.
  • Made it target-specific.
  • Added a diagnostic when it's applied to anything other than a vector type with a vector kind of NeonVector.
  • Added some error tests demonstrating the new diagnostic.
  • Added code examples to the docs.
simon_tatham added inline comments.Jan 14 2020, 9:09 AM
clang/include/clang/Basic/Attr.td
1483

I was going by your recommendation for the spelling of ArmMveAlias in D67159#1659296. I've taken the __clang_ off this one; should I do the same to that one as well, while I'm here?

(It ought to be safe to do that after the fact, because ArmMveAlias is locked down so hard that surely it can't have any users except the tablegen MveEmitter backend, which is easy to change.)

simon_tatham retitled this revision from [clang] New __attribute__((__clang_arm_mve_strict_polymorphism)). to [clang] New __attribute__((arm_mve_strict_polymorphism))..Jan 14 2020, 9:10 AM
simon_tatham edited the summary of this revision. (Show Details)
aaron.ballman accepted this revision.Jan 15 2020, 5:58 AM

Aside from changing the attribute name back to what it was (sorry about that), this LGTM!

clang/include/clang/Basic/Attr.td
1483

Oh, that's right, this is one of those "users should not write this themselves, it's only for compiler internals" attributes. I am sorry for the churn on this review, but we should probably keep the __clang here for consistency with the other attribute. Thank you for reminding me about the prior decision!

This revision is now accepted and ready to land.Jan 15 2020, 5:58 AM
This revision was automatically updated to reflect the committed changes.