Support base vector types of __attribute__((mode))

[alexfrol]

Summary

Base type of attribute((mode)) can actually be a vector type.
The patch is to distinguish between base type and base element type.

This fixes http://llvm.org/PR17453.

[alexfrol]

Aaron Ballman wrote:
"I am unfamiliar with attribute((mode)); is there documentation that supports this assertion, or popular code in the wild that is relying on it? GCC's docs are very spartan on the >topic, and there was an (unanswered) question in the PR regarding why we would want to support this."

attribute((mode)) is a GCC extension. It is already supported in Clang, but only partially as the current implementation does not cover cases with base vector types.
I tend to claim this issue (described in PR17453) a missing feature/incomplete implementation as GCC compiler does support base vector types.
The community already accepted similar patches for this attribute (issues described in PR16752, PR8703 and PR3691, for example). I hope to think this patch is desirable too.

Aaron Ballman wrote: "The diagnostic you added does not accept any arguments."

+    if (ComplexMode) {
+      S.Diag(Attr.getLoc(), diag::err_complex_mode_vector_type) << Name;

Aaron Ballman wrote: "This diagnostic also does not accept any arguments."

+  if (NewTy.isNull()) {
+    S.Diag(Attr.getLoc(), diag::err_mode_wrong_type) << Name;

I removed arguments for the two mentioned diagnostics and updated the patch.

[aaron.ballman]

CC'ing in Chris, who had expressed concerns in the original PR.

[alexfrol]

Ping!

[alexfrol]

Ping!!

[aaron.ballman]

Given the fact that WWDC is happening, I suspect Chris is a bit busy
to chime in with his opinions. ;-)

~Aaron

[alexfrol]

Ping!!

[alexfrol]

Aaron said LGTM, thus I accept the revision.

18.06.2015, 19:26, "Aaron Ballman" <[e-mail]>:

Since Chris isn't strongly opposed, I think the patch LGTM.

~Aaron

On Thu, Jun 18, 2015 at 12:24 PM, Chris Lattner <[e-mail]> wrote:

 On Jun 18, 2015, at 3:02 AM, Alexey Frolov <[e-mail]> wrote:

 Hi Chris,

 Sorry for disturbing :)

 In comment 1 of http://llvm.org/PR17453 you wrote:
   >> It isn't clear that we want to support this. Our goal is not to pass the GCC testsuite.
 What in your opinion should be done with this PR - fixed or closed as "WONTFIX”?

 I don’t have strong opinions about this. On the one hand, I don’t see this as a commonly used extension, so I don’t see it as critical to support. OTOH, if the patch is small and self contained, it is better to be compatible.

 -Chris

 Thank you,
 Alexey Frolov
 =============
 Software Engineer
 Intel Compiler Team
 Intel

cfe/trunk/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 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<
   "'nonnull' attribute when used on parameters takes no arguments">,
   InGroup<IgnoredAttributes>;

cfe/trunk/lib/Sema/SemaDeclAttr.cpp

static void handleModeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
   else if (ValueDecl *VD = dyn_cast<ValueDecl>(D))
     OldTy = VD->getType();
   else {
     S.Diag(D->getLocation(), diag::err_attr_wrong_decl)
       << Attr.getName() << Attr.getRange();
     return;
   }
 
-  if (!OldTy->getAs<BuiltinType>() && !OldTy->isComplexType())
+  // Base type can also be a vector type (see PR17453).
+  // Distinguish between base type and base element type.
+  QualType OldElemTy = OldTy;
+  if (const VectorType *VT = OldTy->getAs<VectorType>())
+    OldElemTy = VT->getElementType();
+
+  if (!OldElemTy->getAs<BuiltinType>() && !OldElemTy->isComplexType())
     S.Diag(Attr.getLoc(), diag::err_mode_not_primitive);
   else if (IntegerMode) {
-    if (!OldTy->isIntegralOrEnumerationType())
+    if (!OldElemTy->isIntegralOrEnumerationType())
       S.Diag(Attr.getLoc(), diag::err_mode_wrong_type);
   } else if (ComplexMode) {
-    if (!OldTy->isComplexType())
+    if (!OldElemTy->isComplexType())
       S.Diag(Attr.getLoc(), diag::err_mode_wrong_type);
   } else {
-    if (!OldTy->isFloatingType())
+    if (!OldElemTy->isFloatingType())
       S.Diag(Attr.getLoc(), diag::err_mode_wrong_type);
   }
 
   // FIXME: Sync this with InitializePredefinedMacros; we need to match int8_t
   // and friends, at least with glibc.
   // FIXME: Make sure floating-point mappings are accurate
   // FIXME: Support XF and TF types
   if (!DestWidth) {
     S.Diag(Attr.getLoc(), diag::err_machine_mode) << 0 /*Unknown*/ << Name;
     return;
   }
 
-  QualType NewTy;
+  QualType NewElemTy;
 
   if (IntegerMode)
-    NewTy = S.Context.getIntTypeForBitwidth(DestWidth,
-                                            OldTy->isSignedIntegerType());
+    NewElemTy = S.Context.getIntTypeForBitwidth(
+        DestWidth, OldElemTy->isSignedIntegerType());
   else
-    NewTy = S.Context.getRealTypeForBitwidth(DestWidth);
+    NewElemTy = S.Context.getRealTypeForBitwidth(DestWidth);
 
-  if (NewTy.isNull()) {
+  if (NewElemTy.isNull()) {
     S.Diag(Attr.getLoc(), diag::err_machine_mode) << 1 /*Unsupported*/ << Name;
     return;
   }
 
   if (ComplexMode) {
-    NewTy = S.Context.getComplexType(NewTy);
+    NewElemTy = S.Context.getComplexType(NewElemTy);
+  }
+
+  QualType NewTy = NewElemTy;
+  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);
+    NewTy =
+        S.Context.getVectorType(NewElemTy, NumElements, OldVT->getVectorKind());
+  }
+
+  if (NewTy.isNull()) {
+    S.Diag(Attr.getLoc(), diag::err_mode_wrong_type);
+    return;
   }
 
   // Install the new type.
   if (TypedefNameDecl *TD = dyn_cast<TypedefNameDecl>(D))
     TD->setModedTypeSourceInfo(TD->getTypeSourceInfo(), NewTy);
   else
     cast<ValueDecl>(D)->setType(NewTy);
 

cfe/trunk/test/CodeGen/attr-mode-vector-types.c

+// RUN: %clang_cc1 -triple x86_64-unknown-unknown %s -emit-llvm -o - | FileCheck %s
+
+typedef int __attribute__((mode(byte))) __attribute__((vector_size(4))) vec_t1;
+typedef int __attribute__((mode(QI))) __attribute__((vector_size(8))) vec_t2;
+typedef int __attribute__((mode(SI))) __attribute__((vector_size(16))) vec_t3;
+typedef int __attribute__((mode(DI))) __attribute__((vector_size(64)))vec_t4;
+typedef float __attribute__((mode(SF))) __attribute__((vector_size(128))) vec_t5;
+typedef float __attribute__((mode(DF))) __attribute__((vector_size(256))) vec_t6;
+
+void check() {
+  // CHECK: alloca <4 x i8>
+  vec_t1 v1;
+  // CHECK: alloca <8 x i8>
+  vec_t2 v2;
+  // CHECK: alloca <4 x i32>
+  vec_t3 v3;
+  // CHECK: alloca <8 x i64>
+  vec_t4 v4;
+  // CHECK: alloca <32 x float>
+  vec_t5 v5;
+  // CHECK: alloca <32 x double>
+  vec_t6 v6;
+}
+
+// CHECK: ret i32 4
+int check_size1() { return sizeof(vec_t1); }
+
+// CHECK: ret i32 8
+int check_size2() { return sizeof(vec_t2); }
+
+// CHECK: ret i32 16
+int check_size3() { return sizeof(vec_t3); }
+
+// CHECK: ret i32 64
+int check_size4() { return sizeof(vec_t4); }
+
+// CHECK: ret i32 128
+int check_size5() { return sizeof(vec_t5); }
+
+// CHECK: ret i32 256
+int check_size6() { return sizeof(vec_t6); }

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

+// RUN: %clang_cc1 -fsyntax-only -verify %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;
+
+// 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'}}