[C++4OpenCL] Fix reinterpret_cast of vectors

[olestrohm]

Summary

This fixes two issues with reinterpret_cast in C++ for OpenCL and adds tests to make sure they both pass without errors, and generate the correct code.

Reinterpret_cast can only convert a type to itself if it is an integral type (or pointer or reference or vectors), so I didn't include any exceptions for opencl types.
Not even float can be converted to itself, so I don't think it makes sense to allow this for any opencl types unlesss you think some of them fit the integral restriction.

Fixes: https://bugs.llvm.org/show_bug.cgi?id=47977 and https://bugs.llvm.org/show_bug.cgi?id=49221

[olestrohm]

Fixed formatting.

[Anastasia]

clang/lib/Sema/SemaCast.cpp

2360

I am thinking of whether we can allow something weird to be written by relaxing this address space case

local int i;
const global int & ii = reinterpret_cast<global int>(i);

I think this code is completely meaningless as it requires creating a temporary in global address space? I am not sure what we will even emit in IR?

Embedded C doesn't regulate the conversions between non-pointer types with different address spaces but in OpenCL C we have allowed them apparently https://godbolt.org/z/9f75zxj7v. I am not sure whether this was done intentionally or not.

However, in C there are no references and other features that might create problems.

At the same time it seems C++ allows casting away const https://godbolt.org/z/fo3axbq3e

But it contradicts the comment below:

// C++ 5.2.10p2 has a note that mentions that, subject to all other
// restrictions, a cast to the same type is allowed so long as it does not
// cast away constness. In C++98, the intent was not entirely clear here,
// since all other paragraphs explicitly forbid casts to the same type.
// C++11 clarifies this case with p2.

I would like to see if @rjmccall has any opinion on whether reinterpreting between non-pointer types with different address spaces is reasonable?

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp

2

Btw I assume your patch also allows reinterpret_cast between vectors? What happens if we cast b/w vector of 3 elements and vector of 4 elements?

[olestrohm]

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp

2

Yes, this is essentially the same as line 10, as int is treated as a 1 element vector, but I can add some more test cases that shows this more specifically.

[Anastasia]

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp

2

Yes, let's add a few test cases with vector to vector casts. I would especially suggest vec3 since it is treated at vec4 in storage I think...

[olestrohm]

Removed the address space related changes. This revision now only allows for reinterpret_cast between vectors of the same size.

Also added tests for two variables that are definitely both vectors.

This now only fixes: https://bugs.llvm.org/show_bug.cgi?id=47977

[Anastasia]

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp

16

I think in OpenCL vector 3 should be setup as vector 4 in memory with the 4th component being undef value. So this conversion should be permitted.

For example you can reinterpret cast between 3 and 4 components vectors using as_typen functions.

https://godbolt.org/z/sKqnYhnGe

I don't think we have a comprehensive description but these are the quotes from OpenCL C:

For 3-component vector data types, the size of the data type is 4 * sizeof(component). This means that a 3-component vector data type will be aligned to a 4 * sizeof(component) boundary. The vload3 and vstore3 built-in functions can be used to read and write, respectively, 3-component vector data types from an array of packed scalar data type.

When the operand and result type contain a different number of elements, the result shall be implementation-defined except if the operand is a 4-component vector and the result is a 3-component vector. In this case, the bits in the operand shall be returned directly without modification as the new type.

The suffixes .lo (or .even) and .hi (or .odd) for a 3-component vector type operate as if the 3-component vector type is a 4-component vector type with the value in the w component undefined.

[olestrohm]

I tried to add a special case for 3 and 4 element vectors, but that caused issues in codgen, so I've left it as a FIXME for now.

[Anastasia]

clang/lib/Sema/SemaCast.cpp

2340

Just a small nit - add . at the end.

clang/lib/Sema/SemaExpr.cpp

7344

Btw I understand that this function is copied over from the old code that has wrong formatting but since we are rewriting this now let's convert it to the current style:
https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

In partucular variable name should start from the upper case letter.

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp

15

Let's add a comment here that it is testing casting OpenCL types to itself.

Btw I assume casting vector to the same vector type will work? Maybe we can add a test case for this too?

At the top of the function we can add a comment about teting the vector type.

[olestrohm]

Fixed the code style and added some comments to the tests.

[Anastasia]

LGTM! Thanks!

Just remember to reflect your final change in the commit message description. :)

clang/include/clang/Sema/Sema.h

QualType CheckMatrixElementwiseOperands(ExprResult &LHS, ExprResult &RHS,
                                           bool IsCompAssign);
   QualType CheckMatrixMultiplyOperands(ExprResult &LHS, ExprResult &RHS,
                                        SourceLocation Loc, bool IsCompAssign);
 
   bool isValidSveBitcast(QualType srcType, QualType destType);
 
   bool areMatrixTypesOfTheSameDimension(QualType srcTy, QualType destTy);
 
+  bool areVectorTypesSameSize(QualType srcType, QualType destType);
   bool areLaxCompatibleVectorTypes(QualType srcType, QualType destType);
   bool isLaxVectorConversion(QualType srcType, QualType destType);
 
   /// type checking declaration initializers (C99 6.7.8)
   bool CheckForConstantInitializer(Expr *e, QualType t);
 
   // type checking C++ declaration initializers (C++ [dcl.init]).
 

clang/lib/Sema/SemaCast.cpp

if (srcIsVector || destIsVector) {
 
     // The size we want to consider is eltCount * eltSize.
     // That's exactly what the lax-conversion rules will check.
     if (Self.areLaxCompatibleVectorTypes(SrcType, DestType)) {
       Kind = CK_BitCast;
       return TC_Success;
     }
 
+    if (Self.LangOpts.OpenCL && !CStyle) {
+      if (DestType->isExtVectorType() || SrcType->isExtVectorType()) {
+        // FIXME: Allow for reinterpret cast between 3 and 4 element vectors
+        if (Self.areVectorTypesSameSize(SrcType, DestType)) {
+          Kind = CK_BitCast;
+          return TC_Success;
+        }
+      }
+    }
+
     // Otherwise, pick a reasonable diagnostic.
     if (!destIsVector)
       msg = diag::err_bad_cxx_cast_vector_to_scalar_different_size;
     else if (!srcIsVector)
       msg = diag::err_bad_cxx_cast_scalar_to_vector_different_size;
     else
       msg = diag::err_bad_cxx_cast_vector_to_vector_different_size;
 
     return TC_Failed;
   }
 
   if (SrcType == DestType) {
     // C++ 5.2.10p2 has a note that mentions that, subject to all other
     // restrictions, a cast to the same type is allowed so long as it does not
     // cast away constness. In C++98, the intent was not entirely clear here,
     // since all other paragraphs explicitly forbid casts to the same type.
     // C++11 clarifies this case with p2.
     //
     // The only allowed types are: integral, enumeration, pointer, or
     // pointer-to-member types.  We also won't restrict Obj-C pointers either.
     Kind = CK_NoOp;

clang/lib/Sema/SemaExpr.cpp

bool Sema::areMatrixTypesOfTheSameDimension(QualType srcTy, QualType destTy) {
 
   const ConstantMatrixType *matSrcType = srcTy->getAs<ConstantMatrixType>();
   const ConstantMatrixType *matDestType = destTy->getAs<ConstantMatrixType>();
 
   return matSrcType->getNumRows() == matDestType->getNumRows() &&
          matSrcType->getNumColumns() == matDestType->getNumColumns();
 }
 
+bool Sema::areVectorTypesSameSize(QualType SrcTy, QualType DestTy) {
+  assert(DestTy->isVectorType() || SrcTy->isVectorType());
+
+  uint64_t SrcLen, DestLen;
+  QualType SrcEltTy, DestEltTy;
+  if (!breakDownVectorType(SrcTy, SrcLen, SrcEltTy))
+    return false;
+  if (!breakDownVectorType(DestTy, DestLen, DestEltTy))
+    return false;
+
+  // ASTContext::getTypeSize will return the size rounded up to a
+  // power of 2, so instead of using that, we need to use the raw
+  // element size multiplied by the element count.
+  uint64_t SrcEltSize = Context.getTypeSize(SrcEltTy);
+  uint64_t DestEltSize = Context.getTypeSize(DestEltTy);
+
+  return (SrcLen * SrcEltSize == DestLen * DestEltSize);
+}
+
 /// Are the two types lax-compatible vector types?  That is, given
 /// that one of them is a vector, do they have equal storage sizes,
 /// where the storage size is the number of elements times the element
 /// size?
 ///
 /// This will also return false if either of the types is neither a
 /// vector nor a real type.
 bool Sema::areLaxCompatibleVectorTypes(QualType srcTy, QualType destTy) {
   assert(destTy->isVectorType() || srcTy->isVectorType());
 
   // Disallow lax conversions between scalars and ExtVectors (these
   // conversions are allowed for other vector types because common headers
   // depend on them).  Most scalar OP ExtVector cases are handled by the
   // splat path anyway, which does what we want (convert, not bitcast).
   // What this rules out for ExtVectors is crazy things like char4*float.
   if (srcTy->isScalarType() && destTy->isExtVectorType()) return false;
   if (destTy->isScalarType() && srcTy->isExtVectorType()) return false;
 
-  uint64_t srcLen, destLen;
-  QualType srcEltTy, destEltTy;
-  if (!breakDownVectorType(srcTy, srcLen, srcEltTy)) return false;
-  if (!breakDownVectorType(destTy, destLen, destEltTy)) return false;
-
-  // ASTContext::getTypeSize will return the size rounded up to a
-  // power of 2, so instead of using that, we need to use the raw
-  // element size multiplied by the element count.
-  uint64_t srcEltSize = Context.getTypeSize(srcEltTy);
-  uint64_t destEltSize = Context.getTypeSize(destEltTy);
-
-  return (srcLen * srcEltSize == destLen * destEltSize);
+  return areVectorTypesSameSize(srcTy, destTy);
 }
 
 /// Is this a legal conversion between two types, one of which is
 /// known to be a vector type?
 bool Sema::isLaxVectorConversion(QualType srcTy, QualType destTy) {
   assert(destTy->isVectorType() || srcTy->isVectorType());
 
   switch (Context.getLangOpts().getLaxVectorConversions()) {

clang/test/CodeGenOpenCLCXX/reinterpret_cast.clcpp

+//RUN: %clang_cc1 %s -triple spir -emit-llvm -O0 -o - | FileCheck %s
+
+typedef int int2 __attribute__((ext_vector_type(2)));
+typedef int int4 __attribute__((ext_vector_type(4)));
+typedef long long2 __attribute__((ext_vector_type(2)));
+
+//CHECK-LABEL: define{{.*}} spir_func void @_Z3barPU3AS1Dv2_i
+void bar(global int2 *in) {
+  //CHECK: bitcast <2 x i32> %{{[0-9]+}} to i64
+  auto l = reinterpret_cast<long>(in[0]);
+  //CHECK: bitcast i64 %{{[0-9]+}} to <2 x i32>
+  auto i2 = reinterpret_cast<int2>(l);
+
+  int4 i4;
+  //CHECK: bitcast <4 x i32> %{{[0-9]+}} to <2 x i64>
+  auto l2 = reinterpret_cast<long2>(i4);
+}

clang/test/SemaOpenCLCXX/reinterpret-cast.clcpp

+// RUN: %clang_cc1 %s -pedantic -verify -fsyntax-only
+
+typedef int int2 __attribute__((ext_vector_type(2)));
+typedef int int3 __attribute__((ext_vector_type(3)));
+typedef int int4 __attribute__((ext_vector_type(4)));
+
+kernel void foo() {
+  // Testing conversions between vectors and vectors/scalars
+  long l1;
+  auto l_to_i2 = reinterpret_cast<int2>(l1);
+  int2 i2;
+  auto i2_to_l = reinterpret_cast<long>(i2);
+  auto i2_to_i = reinterpret_cast<int>(i2); // expected-error{{reinterpret_cast from vector 'int2' (vector of 2 'int' values) to scalar 'int' of different size}}
+  auto i2_to_i2 = reinterpret_cast<int2>(i2);
+
+  // Only integral types (and pointer/references) can be reinterpret casted to themselves.
+  // Currently this does not include any opencl types.
+  reserve_id_t r_id1;
+  auto r_id2 = reinterpret_cast<reserve_id_t>(r_id1); // expected-error{{reinterpret_cast from 'reserve_id_t' to 'reserve_id_t' is not allowed}}
+}