[clang] New __attribute__((arm_mve_strict_polymorphism)).

[simon_tatham]

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

[aaron.ballman]

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

7565

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]

• 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]

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

[aaron.ballman]

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!

clang/include/clang/Basic/Attr.td

 def NeonVectorType : TypeAttr {
   let Spellings = [Clang<"neon_vector_type">];
   let Args = [IntArgument<"NumElements">];
   let Documentation = [Undocumented];
   // Represented as VectorType instead.
   let ASTNode = 0;
 }
 
+def ArmMveStrictPolymorphism : TypeAttr, TargetSpecificAttr<TargetARM> {
+  let Spellings = [Clang<"arm_mve_strict_polymorphism">];
+  let Documentation = [ArmMveStrictPolymorphismDocs];
+}
+
 def NoUniqueAddress : InheritableAttr, TargetSpecificAttr<TargetItaniumCXXABI> {
   let Spellings = [CXX11<"", "no_unique_address", 201803>];
   let Subjects = SubjectList<[NonBitField], ErrorDiag>;
   let Documentation = [NoUniqueAddressDocs];
 }
 
 def ReturnsTwice : InheritableAttr {
   let Spellings = [GCC<"returns_twice">];

clang/include/clang/Basic/AttrDocs.td

 If a function parameter is annotated with `release_handle` it is assumed to
 close the handle. It is also assumed to require an open handle to work with.
 
 .. code-block:: c++
 
   zx_status_t zx_handle_close(zx_handle_t handle [[clang::release_handle]]);
   }];
 }
+
+def ArmMveStrictPolymorphismDocs : Documentation {
+    let Category = DocCatType;
+    let Content = [{
+This attribute is used in the implementation of the ACLE intrinsics for the Arm
+MVE instruction set. It is used to define the vector types used by the MVE
+intrinsics.
+
+Its effect is to modify the behavior of a vector type with respect to function
+overloading. If a candidate function for overload resolution has a parameter
+type with this attribute, then the selection of that candidate function will be
+disallowed if the actual argument can only be converted via a lax vector
+conversion. The aim is to prevent spurious ambiguity in ARM MVE polymorphic
+intrinsics.
+
+.. code-block:: c++
+
+  void overloaded(uint16x8_t vector, uint16_t scalar);
+  void overloaded(int32x4_t vector, int32_t scalar);
+  uint16x8_t myVector;
+  uint16_t myScalar;
+
+  // myScalar is promoted to int32_t as a side effect of the addition,
+  // so if lax vector conversions are considered for myVector, then
+  // the two overloads are equally good (one argument conversion
+  // each). But if the vector has the arm_mve_strict_polymorphism
+  // attribute, only the uint16x8_t,uint16_t overload will match.
+  overloaded(myVector, myScalar + 1);
+
+However, this attribute does not prohibit lax vector conversions in contexts
+other than overloading.
+
+.. code-block:: c++
+
+  uint16x8_t function();
+
+  // This is still permitted with lax vector conversion enabled, even
+  // if the vector types have arm_mve_strict_polymorphism
+  int32x4_t result = function();
+
+    }];
+}

clang/include/clang/Basic/DiagnosticSemaKinds.td

 def warn_objc_unsafe_perform_selector : Warning<
   "%0 is incompatible with selectors that return a "
   "%select{struct|union|vector}1 type">,
   InGroup<DiagGroup<"objc-unsafe-perform-selector">>;
 def note_objc_unsafe_perform_selector_method_declared_here :  Note<
   "method %0 that returns %1 declared here">;
 def err_attribute_arm_mve_alias : Error<
   "'__clang_arm_mve_alias' attribute can only be applied to an ARM MVE builtin">;
+def err_attribute_arm_mve_polymorphism : Error<
+  "'arm_mve_strict_polymorphism' attribute can only be applied to an MVE/NEON vector type">;
 
 def warn_setter_getter_impl_required : Warning<
   "property %0 requires method %1 to be defined - "
   "use @synthesize, @dynamic or provide a method implementation "
   "in this class implementation">,
   InGroup<ObjCPropertyImpl>;
 def warn_setter_getter_impl_required_in_category : Warning<
   "property %0 requires method %1 to be defined - "

clang/lib/AST/TypePrinter.cpp

case attr::PreserveAll:
     OS << "preserve_all";
     break;
   case attr::NoDeref:
     OS << "noderef";
     break;
   case attr::AcquireHandle:
     OS << "acquire_handle";
     break;
+  case attr::ArmMveStrictPolymorphism:
+    OS << "arm_mve_strict_polymorphism";
+    break;
   }
   OS << "))";
 }
 
 void TypePrinter::printObjCInterfaceBefore(const ObjCInterfaceType *T,
                                            raw_ostream &OS) {
   OS << T->getDecl()->getName();
   spaceBeforePlaceHolder(OS);

clang/lib/Sema/SemaOverload.cpp

if (FromType->isArithmeticType()) {
       return true;
     }
   }
 
   // We can perform the conversion between vector types in the following cases:
   // 1)vector types are equivalent AltiVec and GCC vector types
   // 2)lax vector conversions are permitted and the vector types are of the
   //   same size
+  // 3)the destination type does not have the ARM MVE strict-polymorphism
+  //   attribute, which inhibits lax vector conversion for overload resolution
+  //   only
   if (ToType->isVectorType() && FromType->isVectorType()) {
     if (S.Context.areCompatibleVectorTypes(FromType, ToType) ||
-        S.isLaxVectorConversion(FromType, ToType)) {
+        (S.isLaxVectorConversion(FromType, ToType) &&
+         !ToType->hasAttr(attr::ArmMveStrictPolymorphism))) {
       ICK = ICK_Vector_Conversion;
       return true;
     }
   }
 
   return false;
 }
 

clang/lib/Sema/SemaType.cpp

if (vecSize != 64 && vecSize != 128) {
     S.Diag(Attr.getLoc(), diag::err_attribute_bad_neon_vector_size) << CurType;
     Attr.setInvalid();
     return;
   }
 
   CurType = S.Context.getVectorType(CurType, numElts, VecKind);
 }
 
+static void HandleArmMveStrictPolymorphismAttr(TypeProcessingState &State,
+                                               QualType &CurType,
+                                               ParsedAttr &Attr) {
+  const VectorType *VT = dyn_cast<VectorType>(CurType);
+  if (!VT || VT->getVectorKind() != VectorType::NeonVector) {
+    State.getSema().Diag(Attr.getLoc(),
+                         diag::err_attribute_arm_mve_polymorphism);
+    Attr.setInvalid();
+    return;
+  }
+
+  CurType =
+      State.getAttributedType(createSimpleAttr<ArmMveStrictPolymorphismAttr>(
+                                  State.getSema().Context, Attr),
+                              CurType, CurType);
+}
+
 /// Handle OpenCL Access Qualifier Attribute.
 static void HandleOpenCLAccessAttr(QualType &CurType, const ParsedAttr &Attr,
                                    Sema &S) {
   // OpenCL v2.0 s6.6 - Access qualifier can be used only for image and pipe type.
   if (!(CurType->isImageType() || CurType->isPipeType())) {
     S.Diag(Attr.getLoc(), diag::err_opencl_invalid_access_qualifier);
     Attr.setInvalid();
     return;
case ParsedAttr::AT_NeonVectorType:
                                VectorType::NeonVector);
       attr.setUsedAsTypeAttr();
       break;
     case ParsedAttr::AT_NeonPolyVectorType:
       HandleNeonVectorTypeAttr(type, attr, state.getSema(),
                                VectorType::NeonPolyVector);
       attr.setUsedAsTypeAttr();
       break;
+    case ParsedAttr::AT_ArmMveStrictPolymorphism: {
+      HandleArmMveStrictPolymorphismAttr(state, type, attr);
+      attr.setUsedAsTypeAttr();
+      break;
+    }
     case ParsedAttr::AT_OpenCLAccess:
       HandleOpenCLAccessAttr(type, attr, state.getSema());
       attr.setUsedAsTypeAttr();
       break;
     case ParsedAttr::AT_LifetimeBound:
       if (TAL == TAL_DeclChunk)
         HandleLifetimeBoundAttr(state, type, attr);
       break;

clang/test/Sema/overload-arm-mve.c

+// RUN: %clang_cc1 -triple thumbv8.1m.main-arm-none-eabi -target-feature +mve.fp -flax-vector-conversions=all -Werror -emit-llvm -o - %s | FileCheck %s
+// RUN: %clang_cc1 -triple thumbv8.1m.main-arm-none-eabi -target-feature +mve.fp -flax-vector-conversions=all -verify -fsyntax-only -DERROR_CHECK %s
+
+typedef   signed short      int16_t;
+typedef   signed int        int32_t;
+typedef   signed long long  int64_t;
+typedef unsigned short     uint16_t;
+typedef unsigned int       uint32_t;
+typedef unsigned long long uint64_t;
+
+typedef __attribute__((neon_vector_type(8), arm_mve_strict_polymorphism))  int16_t  int16x8_t;
+typedef __attribute__((neon_vector_type(4), arm_mve_strict_polymorphism))  int32_t  int32x4_t;
+typedef __attribute__((neon_vector_type(2), arm_mve_strict_polymorphism))  int64_t  int64x2_t;
+typedef __attribute__((neon_vector_type(8), arm_mve_strict_polymorphism)) uint16_t uint16x8_t;
+typedef __attribute__((neon_vector_type(4), arm_mve_strict_polymorphism)) uint32_t uint32x4_t;
+typedef __attribute__((neon_vector_type(2), arm_mve_strict_polymorphism)) uint64_t uint64x2_t;
+
+__attribute__((overloadable))
+int overload(int16x8_t x, int16_t y); // expected-note {{candidate function}}
+__attribute__((overloadable))
+int overload(int32x4_t x, int32_t y); // expected-note {{candidate function}}
+__attribute__((overloadable))
+int overload(uint16x8_t x, uint16_t y); // expected-note {{candidate function}}
+__attribute__((overloadable))
+int overload(uint32x4_t x, uint32_t y); // expected-note {{candidate function}}
+
+int16_t s16;
+int32_t s32;
+uint16_t u16;
+uint32_t u32;
+
+int16x8_t vs16;
+int32x4_t vs32;
+uint16x8_t vu16;
+uint32x4_t vu32;
+
+// ----------------------------------------------------------------------
+// Simple cases where the types are correctly matched
+
+// CHECK-LABEL: @test_easy_s16(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_int16
+int test_easy_s16(void) { return overload(vs16, s16); }
+
+// CHECK-LABEL: @test_easy_u16(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_uint16
+int test_easy_u16(void) { return overload(vu16, u16); }
+
+// CHECK-LABEL: @test_easy_s32(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_int32
+int test_easy_s32(void) { return overload(vs32, s32); }
+
+// CHECK-LABEL: @test_easy_u32(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_uint32
+int test_easy_u32(void) { return overload(vu32, u32); }
+
+// ----------------------------------------------------------------------
+// Do arithmetic on the scalar, and it may get promoted. We still expect the
+// same overloads to be selected if that happens.
+
+// CHECK-LABEL: @test_promote_s16(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_int16
+int test_promote_s16(void) { return overload(vs16, s16 + 1); }
+
+// CHECK-LABEL: @test_promote_u16(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_uint16
+int test_promote_u16(void) { return overload(vu16, u16 + 1); }
+
+// CHECK-LABEL: @test_promote_s32(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_int32
+int test_promote_s32(void) { return overload(vs32, s32 + 1); }
+
+// CHECK-LABEL: @test_promote_u32(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_uint32
+int test_promote_u32(void) { return overload(vu32, u32 + 1); }
+
+// ----------------------------------------------------------------------
+// Write a simple integer literal without qualification, and expect
+// the vector type to make it unambiguous which integer type you meant
+// the literal to be.
+
+// CHECK-LABEL: @test_literal_s16(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_int16
+int test_literal_s16(void) { return overload(vs16, 1); }
+
+// CHECK-LABEL: @test_literal_u16(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_uint16
+int test_literal_u16(void) { return overload(vu16, 1); }
+
+// CHECK-LABEL: @test_literal_s32(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_int32
+int test_literal_s32(void) { return overload(vs32, 1); }
+
+// CHECK-LABEL: @test_literal_u32(
+// CHECK: call i32 @_Z8overload{{[a-zA-Z0-9_]+}}_uint32
+int test_literal_u32(void) { return overload(vu32, 1); }
+
+// ----------------------------------------------------------------------
+// All of those overload resolutions are supposed to be unambiguous even when
+// lax vector conversion is enabled. Check here that a lax conversion in a
+// different context still works.
+int16x8_t lax_conversion(void) { return vu32; }
+
+// ----------------------------------------------------------------------
+// Use a vector type that there really _isn't_ any overload for, and
+// make sure that we get a fatal compile error.
+
+#ifdef ERROR_CHECK
+int expect_error(uint64x2_t v) {
+  return overload(v, 2); // expected-error {{no matching function for call to 'overload'}}
+}
+
+typedef __attribute__((arm_mve_strict_polymorphism)) int i; // expected-error {{'arm_mve_strict_polymorphism' attribute can only be applied to an MVE/NEON vector type}}
+typedef __attribute__((arm_mve_strict_polymorphism)) int f(); // expected-error {{'arm_mve_strict_polymorphism' attribute can only be applied to an MVE/NEON vector type}}
+typedef __attribute__((arm_mve_strict_polymorphism)) struct { uint16x8_t v; } s; // expected-error {{'arm_mve_strict_polymorphism' attribute can only be applied to an MVE/NEON vector type}}
+#endif

clang/utils/TableGen/MveEmitter.cpp

parts[Float] << "typedef __fp16 float16_t;\n"
                   "typedef float float32_t;\n";
   for (const auto &kv : ScalarTypes) {
     const ScalarType *ST = kv.second.get();
     if (ST->hasNonstandardName())
       continue;
     raw_ostream &OS = parts[ST->requiresFloat() ? Float : 0];
     const VectorType *VT = getVectorType(ST);
 
-    OS << "typedef __attribute__((neon_vector_type(" << VT->lanes() << "))) "
-       << ST->cName() << " " << VT->cName() << ";\n";
+    OS << "typedef __attribute__((__neon_vector_type__(" << VT->lanes()
+       << "), __arm_mve_strict_polymorphism__)) " << ST->cName() << " "
+       << VT->cName() << ";\n";
 
     // Every vector type also comes with a pair of multi-vector types for
     // the VLD2 and VLD4 instructions.
     for (unsigned n = 2; n <= 4; n += 2) {
       const MultiVectorType *MT = getMultiVectorType(n, VT);
       OS << "typedef struct { " << VT->cName() << " val[" << n << "]; } "
          << MT->cName() << ";\n";
     }