{Sema] Gcc compatibility of vector shift.

[vbyakovlcl]

Summary

Gcc prints error if elements of left and right parts of a shift have different sizes. This patch is provided the GCC compatibility.

[aaron.ballman]

In clang 3.8, your test code already produces a diagnostic for me: http://coliru.stacked-crooked.com/a/752a4ea34123bdae

[vbyakovlcl]

Clang 3.8 balances vector shift operand erroneous using CheckVectorOperands which converts one of operand to the type of another. In D21678 it was fixed by using checkVectorShift instead. As result clang does not emit error if shift operands have different element sizes (bat gcc does).

[aaron.ballman]

In D24669#548984, @vbyakovlcl wrote:

Clang 3.8 balances vector shift operand erroneous using CheckVectorOperands which converts one of operand to the type of another. In D21678 it was fixed by using checkVectorShift instead. As result clang does not emit error if shift operands have different element sizes (bat gcc does).

Ah, thank you for the explanation!

llvm/tools/clang/include/clang/Basic/DiagnosticGroups.td

522 ↗

(On Diff #71646)

Is this the same warning flag GCC uses?

llvm/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

2306 ↗

(On Diff #71646)

Why is this off by default?

[vbyakovlcl]

llvm/tools/clang/include/clang/Basic/DiagnosticGroups.td

522 ↗

(On Diff #71646)

Gcc prints error messages

t.c:21:13: error: invalid operands to binary << (have vector_int8 and vector_short8)

vi8 = vi8 << vs8;

I could not find a flag controlling this error.

llvm/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

2306 ↗

(On Diff #71646)

The question is: would we like to have the feature as a clang extension?

[aaron.ballman]

llvm/tools/clang/include/clang/Basic/DiagnosticGroups.td

522 ↗

(On Diff #71646)

I would not add this as a diagnostic group, but instead use an ad-hoc group on the diagnostic itself. I don't think this is going to see very many diagnostics covered by the same group, but if that turns out to be the case, we can switch then.

llvm/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

2306 ↗

(On Diff #71646)

I'm not the best one to answer that question, but we typically avoid adding new off-by-default diagnostics. Since GCC prints this as an error message and this patch is for GCC compatibility, it seems weird to me to add this as an off-by-default warning.

[vbyakovlcl]

llvm/tools/clang/include/clang/Basic/DiagnosticGroups.td

522 ↗

(On Diff #71646)

Ok, done

llvm/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

2306 ↗

(On Diff #71646)

I did similar to GCC - error by default.

[ahatanak]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73468)

Is it possible to use ASTContext::getTypeSize here?

[vbyakovlcl]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73468)

You are right.

[bruno]

llvm/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

2306 ↗

(On Diff #73642)

Although the motivation is to support the same warning present in GCC, I think this is helpful enough anyway so that we might skip calling it "gnu-vec-elem-size" and have a more generic name instead? How about plain "vec-elem-size"?

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73642)

Besides __ext_vector_type__, would this also trigger for vector_size? Right now this is an error for vector_size primarily because the number of elements is different, can you confirm this won't change?

[vbyakovlcl]

llvm/tools/clang/include/clang/Basic/DiagnosticSemaKinds.td

2306 ↗

(On Diff #73642)

I'm agree.

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73642)

I compare vector element sizes, so there must not be any differencies caused by different triggers. I added additional type definitions to the tests. All compiles fain.

[bruno]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73642)

I don't think this is right. When I try to compile similar code for vector_size without your patch, I get the errors:

/tmp/x.c:80:15: error: vector operands do not have the same number of elements ('vector_int8n' (vector of 2 'int' values) and 'vector_uchar8n' (vector of 8 'unsigned char' values))

vi8n = vi8n << vuc8n; // expected-warning {{vector operands do not have the same elements sizes}}
       ~~~~ ^  ~~~~~

/tmp/x.c:81:17: error: vector operands do not have the same number of elements ('vector_uchar8n' (vector of 8 'unsigned char' values) and 'vector_int8n' (vector of 2 'int' values))

vuc8n = vuc8n << vi8n; // expected-warning {{vector operands do not have the same elements sizes}}
        ~~~~~ ^  ~~~~

/tmp/x.c:82:17: error: vector operands do not have the same number of elements ('vector_ushort8n' (vector of 4 'unsigned short' values) and 'vector_uint8n' (vector of 2 'unsigned int' values))

vus8n = vus8n << vui8n; // expected-warning {{vector operands do not have the same elements sizes}}
        ~~~~~ ^  ~~~~~

/tmp/x.c:83:17: error: vector operands do not have the same number of elements ('vector_uint8n' (vector of 2 'unsigned int' values) and 'vector_short8n' (vector of 4 'short' values))

vui8n = vui8n << vs8n; // expected-warning {{vector operands do not have the same elements sizes}}
        ~~~~~ ^  ~~~~

Given your test changes, it seems that now, instead of "vector operands do not have the same number of elements" we would get "vector operands do not have the same elements sizes". I rather we stay with the first. Additionally, even if we had "vector operands do not have the same elements sizes" for vector_size, this should never be demoted to a warning.

[majnemer]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8795–8798 ↗

(On Diff #74123)

Why return QualType() here? Would returning LHSType provide confusing or incorrect diagnostics?

[vbyakovlcl]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8795–8798 ↗

(On Diff #74123)

This is really excessive. Thanks.

8787 ↗

(On Diff #73642)

Argument of a GNU vector size attribute specifies vector size measured in bytes(see https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html). So you got right diagnostics. Both compilers with and without my changes print the same diagnostics for yours case. Here is a small testcase used both GNU and clang extensions

$ cat bruno.c
typedef attribute((ext_vector_type(8))) int vector_int8;
typedef attribute((ext_vector_type(8))) unsigned char vector_uchar8;

typedef attribute((vector_size(8))) int vector_int8n;
typedef attribute((vector_size(8))) unsigned char vector_uchar8n;

vector_int8 vi8;
vector_uchar8 vuc8;
vector_int8n vi8n;
vector_uchar8n vuc8n;

int foo() {

vi8 = vi8 << vuc8;
vi8n = vi8n << vuc8n;

$ clang -c bruno.c -Wno-error-vec-elem-size
bruno.c:13:13: warning: vector operands do not have the same elements sizes ('vector_int8' (vector of 8 'int' values) and 'vector_uchar8' (vector of 8 'unsigned char' values)) [-Wvec-elem-size]

vi8 = vi8 << vuc8;
      ~~~ ^  ~~~~

bruno.c:14:15: error: vector operands do not have the same number of elements ('vector_int8n' (vector of 2 'int' values) and 'vector_uchar8n' (vector of 8 'unsigned char' values))

vi8n = vi8n << vuc8n;

The compiler without the changes prints the second error only (bruno.c:14:15).

[bruno]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73642)

What actually concerns me here is the following: if you invoke clang -c bruno.c -Wvec-elem-size, will that override the error: vector operands do not have the same number of elements message for vector_size typed vectors? If so, I don't think this is right.

[vbyakovlcl]

llvm/tools/clang/lib/Sema/SemaExpr.cpp

8787 ↗

(On Diff #73642)

No, this will not override the error because these diagnostics use independent conditions. The option vec-elem-size is used only for controlling kind of message 'vector_element_sizes_not_equal' - warning or error. Using this flag cannot influence on the massage you worry.

$ clang bruno.c -c -Wvec-elem-size
bruno.c:13:13: error: vector operands do not have the same elements sizes ('vector_int8' (vector of 8 'int' values) and 'vector_uchar8' (vector of 8 'unsigned char' values)) [-Wvec-elem-size]

vi8 = vi8 << vuc8;
      ~~~ ^  ~~~~

bruno.c:14:15: error: vector operands do not have the same number of elements ('vector_int8n' (vector of 2 'int' values) and 'vector_uchar8n' (vector of 8 'unsigned char' values))

vi8n = vi8n << vuc8n;
       ~~~~ ^  ~~~~~

2 errors generated.

$ clang bruno.c -c -Wno-vec-elem-size
bruno.c:14:15: error: vector operands do not have the same number of elements ('vector_int8n' (vector of 2 'int' values) and 'vector_uchar8n' (vector of 8 'unsigned char' values))

vi8n = vi8n << vuc8n;
       ~~~~ ^  ~~~~~

1 error generated.
$ clang bruno.c -c -Wno-error-vec-elem-size
bruno.c:13:13: warning: vector operands do not have the same elements sizes ('vector_int8' (vector of 8 'int' values) and 'vector_uchar8' (vector of 8 'unsigned char' values)) [-Wvec-elem-size]

vi8 = vi8 << vuc8;
      ~~~ ^  ~~~~

bruno.c:14:15: error: vector operands do not have the same number of elements ('vector_int8n' (vector of 2 'int' values) and 'vector_uchar8n' (vector of 8 'unsigned char' values))

vi8n = vi8n << vuc8n;
       ~~~~ ^  ~~~~~

1 warning and 1 error generated.

[bruno]

Ok, great!

LGTM

cfe/trunk/include/clang/Basic/DiagnosticSemaKinds.td

 def err_attribute_zero_size : Error<"zero vector size">;
 def err_attribute_size_too_large : Error<"vector size too large">;
 def err_typecheck_vector_not_convertable : Error<
   "cannot convert between vector values of different size (%0 and %1)">;
 def err_typecheck_vector_not_convertable_non_scalar : Error<
   "cannot convert between vector and non-scalar values (%0 and %1)">;
 def err_typecheck_vector_lengths_not_equal : Error<
   "vector operands do not have the same number of elements (%0 and %1)">;
+def warn_typecheck_vector_element_sizes_not_equal : Warning<
+  "vector operands do not have the same elements sizes (%0 and %1)">,
+  InGroup<DiagGroup<"vec-elem-size">>, DefaultError;
 def err_ext_vector_component_exceeds_length : Error<
   "vector component access exceeds type %0">;
 def err_ext_vector_component_name_illegal : Error<
   "illegal vector component name '%0'">;
 def err_attribute_address_space_negative : Error<
   "address space is negative">;
 def err_attribute_address_space_too_high : Error<
   "address space is larger than the maximum supported (%0)">;

cfe/trunk/lib/Sema/SemaExpr.cpp

if (!LHSVecTy) {
     // are applied component-wise. So if RHS is a vector, then ensure
     // that the number of elements is the same as LHS...
     if (RHSVecTy->getNumElements() != LHSVecTy->getNumElements()) {
       S.Diag(Loc, diag::err_typecheck_vector_lengths_not_equal)
         << LHS.get()->getType() << RHS.get()->getType()
         << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
       return QualType();
     }
+    if (!S.LangOpts.OpenCL && !S.LangOpts.ZVector) {
+      const BuiltinType *LHSBT = LHSEleType->getAs<clang::BuiltinType>();
+      const BuiltinType *RHSBT = RHSEleType->getAs<clang::BuiltinType>();
+      if (LHSBT != RHSBT &&
+          S.Context.getTypeSize(LHSBT) != S.Context.getTypeSize(RHSBT)) {
+        S.Diag(Loc, diag::warn_typecheck_vector_element_sizes_not_equal)
+            << LHS.get()->getType() << RHS.get()->getType()
+            << LHS.get()->getSourceRange() << RHS.get()->getSourceRange();
+      }
+    }
   } else {
     // ...else expand RHS to match the number of elements in LHS.
     QualType VecTy =
       S.Context.getExtVectorType(RHSEleType, LHSVecTy->getNumElements());
     RHS = S.ImpCastExprToType(RHS.get(), VecTy, CK_VectorSplat);
   }
 
   return LHSType;

cfe/trunk/test/CodeGen/vecshift.c

-// RUN: %clang_cc1 -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1  -Wno-error-vec-elem-size -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang_cc1  -Wno-error-vec-elem-size -DEXT -emit-llvm %s -o - | FileCheck %s
 
+#ifdef EXT
 typedef __attribute__((__ext_vector_type__(8))) char vector_char8;
 typedef __attribute__((__ext_vector_type__(8))) short vector_short8;
 typedef __attribute__((__ext_vector_type__(8))) int vector_int8;
 typedef __attribute__((__ext_vector_type__(8))) unsigned char vector_uchar8;
 typedef __attribute__((__ext_vector_type__(8))) unsigned short vector_ushort8;
 typedef __attribute__((__ext_vector_type__(8))) unsigned int vector_uint8;
 typedef __attribute__((__ext_vector_type__(4))) char vector_char4;
 typedef __attribute__((__ext_vector_type__(4))) short vector_short4;
 typedef __attribute__((__ext_vector_type__(4))) int vector_int4;
 typedef __attribute__((__ext_vector_type__(4))) unsigned char vector_uchar4;
 typedef __attribute__((__ext_vector_type__(4))) unsigned short vector_ushort4;
 typedef __attribute__((__ext_vector_type__(4))) unsigned int vector_uint4;
+#else
+typedef __attribute__((vector_size(8))) char vector_char8;
+typedef __attribute__((vector_size(16))) short vector_short8;
+typedef __attribute__((vector_size(32))) int vector_int8;
+typedef __attribute__((vector_size(8))) unsigned char vector_uchar8;
+typedef __attribute__((vector_size(16))) unsigned short vector_ushort8;
+typedef __attribute__((vector_size(32))) unsigned int vector_uint8;
+typedef __attribute__((vector_size(4))) char vector_char4;
+typedef __attribute__((vector_size(4))) short vector_short4;
+typedef __attribute__((vector_size(16))) int vector_int4;
+typedef __attribute__((vector_size(4))) unsigned char vector_uchar4;
+typedef __attribute__((vector_size(8))) unsigned short vector_ushort4;
+typedef __attribute__((vector_size(16))) unsigned int vector_uint4;
+#endif
 
 char c;
 short s;
 int i;
 unsigned char uc;
 unsigned short us;
 unsigned int ui;
 vector_char8 vc8;

cfe/trunk/test/Sema/vecshift.c

-// RUN: %clang_cc1 -fsyntax-only -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DERR -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wno-error-vec-elem-size -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DEXT -DERR -verify %s
+// RUN: %clang_cc1 -fsyntax-only -DEXT -Wno-error-vec-elem-size -verify %s
 
+#ifdef EXT
 typedef __attribute__((__ext_vector_type__(8))) char vector_char8;
 typedef __attribute__((__ext_vector_type__(8))) short vector_short8;
 typedef __attribute__((__ext_vector_type__(8))) int vector_int8;
 typedef __attribute__((__ext_vector_type__(8))) unsigned char vector_uchar8;
 typedef __attribute__((__ext_vector_type__(8))) unsigned short vector_ushort8;
 typedef __attribute__((__ext_vector_type__(8))) unsigned int vector_uint8;
 typedef __attribute__((__ext_vector_type__(4))) char vector_char4;
 typedef __attribute__((__ext_vector_type__(4))) short vector_short4;
 typedef __attribute__((__ext_vector_type__(4))) int vector_int4;
 typedef __attribute__((__ext_vector_type__(4))) unsigned char vector_uchar4;
 typedef __attribute__((__ext_vector_type__(4))) unsigned short vector_ushort4;
 typedef __attribute__((__ext_vector_type__(4))) unsigned int vector_uint4;
+#else
+typedef __attribute__((vector_size(8))) char vector_char8;
+typedef __attribute__((vector_size(16))) short vector_short8;
+typedef __attribute__((vector_size(32))) int vector_int8;
+typedef __attribute__((vector_size(8))) unsigned char vector_uchar8;
+typedef __attribute__((vector_size(16))) unsigned short vector_ushort8;
+typedef __attribute__((vector_size(32))) unsigned int vector_uint8;
+typedef __attribute__((vector_size(4))) char vector_char4;
+typedef __attribute__((vector_size(4))) short vector_short4;
+typedef __attribute__((vector_size(16))) int vector_int4;
+typedef __attribute__((vector_size(4))) unsigned char vector_uchar4;
+typedef __attribute__((vector_size(8))) unsigned short vector_ushort4;
+typedef __attribute__((vector_size(16))) unsigned int vector_uint4;
+#endif
 
 char c;
 short s;
 int i;
 unsigned char uc;
 unsigned short us;
 unsigned int ui;
 vector_char8 vc8;
void foo() {
   vc8 = c << vc8;
   vuc8 = i << vuc8;
   vi8 = uc << vi8;
   vui8 = us << vui8;
   vs8 = ui << vs8;
   vus8 = 1 << vus8;
 
   vc8 = vc8 << vc8;
-  vi8 = vi8 << vuc8;
-  vuc8 = vuc8 << vi8;
-  vus8 = vus8 << vui8;
-  vui8 = vui8 << vs8;
+#ifdef ERR
+  vi8 = vi8 << vuc8; // expected-error {{vector operands do not have the same elements sizes}}
+  vuc8 = vuc8 << vi8; // expected-error {{vector operands do not have the same elements sizes}}
+  vus8 = vus8 << vui8; // expected-error {{vector operands do not have the same elements sizes}}
+  vui8 = vui8 << vs8; // expected-error {{vector operands do not have the same elements sizes}}
+#else
+  vi8 = vi8 << vuc8; // expected-warning {{vector operands do not have the same elements sizes}}
+  vuc8 = vuc8 << vi8; // expected-warning {{vector operands do not have the same elements sizes}}
+  vus8 = vus8 << vui8; // expected-warning {{vector operands do not have the same elements sizes}}
+  vui8 = vui8 << vs8; // expected-warning {{vector operands do not have the same elements sizes}}
+#endif
 
   vc8 <<= vc8;
-  vi8 <<= vuc8;
-  vuc8 <<= vi8;
-  vus8 <<= vui8;
-  vui8 <<= vs8;
+#ifdef ERR
+  vi8 <<= vuc8; // expected-error {{vector operands do not have the same elements sizes}}
+  vuc8 <<= vi8; // expected-error {{vector operands do not have the same elements sizes}}
+  vus8 <<= vui8; // expected-error {{vector operands do not have the same elements sizes}}
+  vui8 <<= vs8; // expected-error {{vector operands do not have the same elements sizes}}
+#else
+  vi8 <<= vuc8; // expected-warning {{vector operands do not have the same elements sizes}}
+  vuc8 <<= vi8; // expected-warning {{vector operands do not have the same elements sizes}}
+  vus8 <<= vui8; // expected-warning {{vector operands do not have the same elements sizes}}
+  vui8 <<= vs8; // expected-warning {{vector operands do not have the same elements sizes}}
+#endif
 
   c <<= vc8; // expected-error {{assigning to 'char' from incompatible type}}
   i <<= vuc8; // expected-error {{assigning to 'int' from incompatible type}}
   uc <<= vi8; // expected-error {{assigning to 'unsigned char' from incompatible type}}
   us <<= vui8; // expected-error {{assigning to 'unsigned short' from incompatible type}}
   ui <<= vs8; // expected-error {{assigning to 'unsigned int' from incompatible type}}
 }