PR10235: support for vector mode attributes + warning

[DmitryPolukhin]

Summary

Add support for vector mode attributes like "attribute((mode(V4SF)))". Also add warning about deprecated vector modes like GCC does.

[aaron.ballman]

I am a little bit confused -- this patch adds a deprecation warning for vector sizes and claims that the attribute is ignored (by placing it in the IgnoredAttributes group), but then provides the initial implementation for this functionality and makes use of the attribute. If it's deprecated, why implement the behavior? Better to say "we recognize this attribute and won't explode on it, but do not implement the functionality since it is deprecated."

include/clang/Basic/DiagnosticSemaKinds.td

2765

How about:

"'mode' attribute is not supported for vector types; use the 'vector_size' attribute instead"

[DmitryPolukhin]

Thank you for prompt response!

include/clang/Basic/DiagnosticSemaKinds.td

2765

Both GCC and ICC support vector types in mode attribute with similar warning so for compatibility reasons it is better to implement it. Taking into account that detection of vector types is more than half of supporting them. I do agree that warning group is confusing. I was not able to find more suitable warning group so would you recommend to create new one for this warning?

[aaron.ballman]

include/clang/Basic/DiagnosticSemaKinds.td

2765

Are there headers from major library vendors that rely on supporting this feature? If not, then I would say it's probably better to simply not implement the diagnostic and fail to support such constructs at all. If it is used by major vendors and would prevent otherwise functional code from working, then this approach is reasonable. In that case, I would add a new diagnostic group (DeprecatedAttributes and add it to the Deprecated group).

[DmitryPolukhin]

Added new warning group as suggested.

I see thousands of vector types in headers on GitHub https://github.com/search?p=1&q=%22__attribute__+mode+v4sf%22+extension%3Ah&ref=searchresults&type=Code&utf8=%E2%9C%93

liboil from freedesktop.org uses vector types in headers.

[aaron.ballman]

In D14744#291663, @DmitryPolukhin wrote:

Added new warning group as suggested.

I see thousands of vector types in headers on GitHub https://github.com/search?p=1&q=%22__attribute__+mode+v4sf%22+extension%3Ah&ref=searchresults&type=Code&utf8=%E2%9C%93

liboil from freedesktop.org uses vector types in headers.

The vast majority of those headers all appear to be GCC's vector-defs.h, so I'm still not convinced this is actually something we need to solve. However, I also don't see the harm.

include/clang/Basic/DiagnosticSemaKinds.td

2765

Should be reworded slightly to be consistent with other attribute diagnostics:

"specifying vector types with the 'mode' attribute is deprecated; use the 'vector_size' attribute instead"

lib/Sema/SemaDeclAttr.cpp

3220

parseModeAttrArg() please.

3296

Can you have a vector of vectors with mode? I'm wondering why this isn't part of parseModeAttr(), since this is parsing part of the mode attribute.

3299

This will cause a buffer overrun on pathological code; should also constrain based on the size of Str.

[DmitryPolukhin]

lib/Sema/SemaDeclAttr.cpp

3296

No, it is not possible and more over it is not allowed to combine vector_size and vector mode (GCC does the same). Checks below prevent non-primitive types as vector elements.

3299

Thank you for catching this!

[aaron.ballman]

Thanks, this LGTM!

~Aaron

include/clang/Basic/DiagnosticSemaKinds.td

def warn_ignored_ms_inheritance : Warning<
   InGroup<IgnoredAttributes>;
 def note_previous_ms_inheritance : Note<
   "previous inheritance model specified here">;
 def err_machine_mode : Error<"%select{unknown|unsupported}0 machine mode %1">;
 def err_mode_not_primitive : Error<
   "mode attribute only supported for integer and floating-point types">;
 def err_mode_wrong_type : Error<
   "type of machine mode does not match type of base type">;
+def warn_vector_mode_deprecated : Warning<
+  "specifying vector types with __attribute__ ((mode)) is deprecated, "
+  "use __attribute__ ((vector_size)) instead">,
+  InGroup<IgnoredAttributes>;
 def err_complex_mode_vector_type : Error<
   "type of machine mode does not support base vector types">;
 def err_attr_wrong_decl : Error<
   "%0 attribute invalid on this declaration, requires typedef or value">;
 def warn_attribute_nonnull_no_pointers : Warning<
   "'nonnull' attribute applied to function with no pointer arguments">,
   InGroup<IgnoredAttributes>;
 def warn_attribute_nonnull_parm_no_args : Warning<

lib/Sema/SemaDeclAttr.cpp

bool Sema::checkMSInheritanceAttrOnDefinition(
 
   Diag(Range.getBegin(), diag::err_mismatched_ms_inheritance)
       << 0 /*definition*/;
   Diag(RD->getDefinition()->getLocation(), diag::note_defined_here)
       << RD->getNameAsString();
   return true;
 }
 
-/// handleModeAttr - This attribute modifies the width of a decl with primitive
-/// type.
-///
-/// Despite what would be logical, the mode attribute is a decl attribute, not a
-/// type attribute: 'int ** __attribute((mode(HI))) *G;' tries to make 'G' be
-/// HImode, not an intermediate pointer.
-static void handleModeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
-  // This attribute isn't documented, but glibc uses it.  It changes
-  // the width of an int or unsigned int to the specified size.
-  if (!Attr.isArgIdent(0)) {
-    S.Diag(Attr.getLoc(), diag::err_attribute_argument_type) << Attr.getName()
-      << AANT_ArgumentIdentifier;
-    return;
-  }
-  
-  IdentifierInfo *Name = Attr.getArgAsIdent(0)->Ident;
-  StringRef Str = Name->getName();
-
-  normalizeName(Str);
-
-  unsigned DestWidth = 0;
-  bool IntegerMode = true;
-  bool ComplexMode = false;
+/// parseModeAttr - Parses attribute mode string and returns parsed type
+/// attribute.
+static void parseModeAttr(Sema &S, StringRef Str, unsigned &DestWidth,
+                          bool &IntegerMode, bool &ComplexMode) {
   switch (Str.size()) {
   case 2:
     switch (Str[0]) {
-    case 'Q': DestWidth = 8; break;
-    case 'H': DestWidth = 16; break;
-    case 'S': DestWidth = 32; break;
-    case 'D': DestWidth = 64; break;
-    case 'X': DestWidth = 96; break;
-    case 'T': DestWidth = 128; break;
+    case 'Q':
+      DestWidth = 8;
+      break;
+    case 'H':
+      DestWidth = 16;
+      break;
+    case 'S':
+      DestWidth = 32;
+      break;
+    case 'D':
+      DestWidth = 64;
+      break;
+    case 'X':
+      DestWidth = 96;
+      break;
+    case 'T':
+      DestWidth = 128;
+      break;
     }
     if (Str[1] == 'F') {
       IntegerMode = false;
     } else if (Str[1] == 'C') {
       IntegerMode = false;
       ComplexMode = true;
     } else if (Str[1] != 'I') {
       DestWidth = 0;
case 7:
     if (Str == "pointer")
       DestWidth = S.Context.getTargetInfo().getPointerWidth(0);
     break;
   case 11:
     if (Str == "unwind_word")
       DestWidth = S.Context.getTargetInfo().getUnwindWordWidth();
     break;
   }
+}
+
+/// handleModeAttr - This attribute modifies the width of a decl with primitive
+/// type.
+///
+/// Despite what would be logical, the mode attribute is a decl attribute, not a
+/// type attribute: 'int ** __attribute((mode(HI))) *G;' tries to make 'G' be
+/// HImode, not an intermediate pointer.
+static void handleModeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  // This attribute isn't documented, but glibc uses it.  It changes
+  // the width of an int or unsigned int to the specified size.
+  if (!Attr.isArgIdent(0)) {
+    S.Diag(Attr.getLoc(), diag::err_attribute_argument_type) << Attr.getName()
+      << AANT_ArgumentIdentifier;
+    return;
+  }
+
+  IdentifierInfo *Name = Attr.getArgAsIdent(0)->Ident;
+  StringRef Str = Name->getName();
+
+  normalizeName(Str);
+
+  unsigned DestWidth = 0;
+  bool IntegerMode = true;
+  bool ComplexMode = false;
+  llvm::APInt VectorSize(64, 0);
+  if (Str.size() >= 4 && Str[0] == 'V') {
+    // Minimal length of vector mode is 4: 'V' + NUMBER(>=1) + TYPE(>=2).
+    int VectorStringLength = 0;
+    while (isdigit(Str[VectorStringLength + 1]))
+      ++VectorStringLength;
+    if (VectorStringLength &&
+        !Str.substr(1, VectorStringLength).getAsInteger(10, VectorSize) &&
+        VectorSize.isPowerOf2()) {
+      parseModeAttr(S, Str.substr(VectorStringLength + 1), DestWidth,
+                    IntegerMode, ComplexMode);
+      S.Diag(Attr.getLoc(), diag::warn_vector_mode_deprecated);
+    } else {
+      VectorSize = 0;
+    }
+  }
+
+  if (!VectorSize)
+    parseModeAttr(S, Str, DestWidth, IntegerMode, ComplexMode);
 
   QualType OldTy;
   if (TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D))
     OldTy = TD->getUnderlyingType();
   else if (ValueDecl *VD = dyn_cast<ValueDecl>(D))
     OldTy = VD->getType();
   else {
     S.Diag(D->getLocation(), diag::err_attr_wrong_decl)
if (NewElemTy.isNull()) {
     return;
   }
 
   if (ComplexMode) {
     NewElemTy = S.Context.getComplexType(NewElemTy);
   }
 
   QualType NewTy = NewElemTy;
-  if (const VectorType *OldVT = OldTy->getAs<VectorType>()) {
+  if (VectorSize.getBoolValue()) {
+    NewTy = S.Context.getVectorType(NewTy, VectorSize.getZExtValue(),
+                                    VectorType::GenericVector);
+  } else if (const VectorType *OldVT = OldTy->getAs<VectorType>()) {
     // Complex machine mode does not support base vector types.
     if (ComplexMode) {
       S.Diag(Attr.getLoc(), diag::err_complex_mode_vector_type);
       return;
     }
     unsigned NumElements = S.Context.getTypeSize(OldElemTy) *
                            OldVT->getNumElements() /
                            S.Context.getTypeSize(NewElemTy);

test/Sema/attr-mode-vector-types.c

 // RUN: %clang_cc1 -fsyntax-only -verify -triple x86_64-unknown-linux-gnu %s
 
 // Correct cases.
 typedef int __attribute__((mode(byte))) __attribute__((vector_size(256))) vec_t1;
 typedef int __attribute__((mode(QI))) __attribute__((vector_size(256))) vec_t2;
 typedef int __attribute__((mode(SI))) __attribute__((vector_size(256))) vec_t3;
 typedef int __attribute__((mode(DI))) __attribute__((vector_size(256)))vec_t4;
 typedef float __attribute__((mode(SF))) __attribute__((vector_size(256))) vec_t5;
 typedef float __attribute__((mode(DF))) __attribute__((vector_size(256))) vec_t6;
 typedef float __attribute__((mode(XF))) __attribute__((vector_size(256))) vec_t7;
 
+typedef int v8qi __attribute__ ((mode(QI))) __attribute__ ((vector_size(8)));
+typedef int v8qi __attribute__ ((mode(V8QI)));
+// expected-warning@-1{{specifying vector types with __attribute__ ((mode)) is deprecated, use __attribute__ ((vector_size)) instead}}
+
+typedef float v4sf __attribute__((mode(V4SF)));
+// expected-warning@-1{{specifying vector types with __attribute__ ((mode)) is deprecated, use __attribute__ ((vector_size)) instead}}
+typedef float v4sf __attribute__((mode(SF)))  __attribute__ ((vector_size(16)));
+
 // Incorrect cases.
 typedef float __attribute__((mode(QC))) __attribute__((vector_size(256))) vec_t8;
 // expected-error@-1{{unsupported machine mode 'QC'}}
 // expected-error@-2{{type of machine mode does not match type of base type}}
 typedef _Complex float __attribute__((mode(HC))) __attribute__((vector_size(256))) vec_t9;
 // expected-error@-1{{unsupported machine mode 'HC'}}
 // expected-error@-2{{invalid vector element type '_Complex float'}}
 typedef int __attribute__((mode(SC))) __attribute__((vector_size(256))) vec_t10;
 // expected-error@-1{{type of machine mode does not match type of base type}}
 // expected-error@-2{{type of machine mode does not support base vector types}}
 typedef float __attribute__((mode(DC))) __attribute__((vector_size(256))) vec_t11;
 // expected-error@-1{{type of machine mode does not match type of base type}}
 // expected-error@-2{{type of machine mode does not support base vector types}}
 typedef _Complex float __attribute__((mode(XC))) __attribute__((vector_size(256))) vec_t12;
 // expected-error@-1{{invalid vector element type '_Complex float'}}
+typedef int __attribute__((mode(V3QI))) v3qi;
+// expected-error@-1{{unknown machine mode 'V3QI'}}