[Sema] Support lax conversions for compound assignments

[bruno]

Summary

Support lax conversions on compound assignment expressions like:

typedef __attribute__((vector_size(8))) double float64x1_t;
typedef __attribute__((vector_size(16))) double float64x2_t;
float64x1_t vget_low_f64(float64x2_t __p0);

double c = 3.0;
float64x2_t v = {0.0, 1.0};
c += vget_low_f64(v);

This restores one more valid behavior pre r266366, and is a incremental
follow up from work committed in r274646.

While here, make the check more strict, add FIXMEs, clean up variable
names to match what they can actually be and update testcase to reflect
that. We now reject:

typedef float float2 __attribute__ ((vector_size (8)));
double d;
f2 += d;

which doesn't fit as a direct bitcast anyway.

[bruno]

Ping!

[ahatanak]

lib/Sema/SemaExpr.cpp

8090

My understanding is that, when we have a compound assign like "LHS += RHS", this function (CheckVectorOperands) is supposed to return the result type (LHS + RHS). However, it is returning different types for "<1 x T> += T" and "T += <1 x T>" (the former returns <1 x T> and the latter returns T). Would CheckAssignmentOperands reject the compound statement if you returned the vector type here?

Also, are you planning to allow the same kind of conversions done above for non-compound assignment statements (e.g., <4 x short> += <2 x int>) in the future?

[bruno]

lib/Sema/SemaExpr.cpp

8090

1. CheckAssignmentOperands doesn't reject the compound statement if we return the vector type instead, because it already supports vector to scalar cast idiom. It makes more sense to return a vector indeed, gonna change that.

2. It would be nice to catch up with GCC with the idioms supported for regular (non-ext) vectors (like splatting scalar operands to vectors in a arith binops), and those, AFAIK, don't include support for truncation as in the example you suggested. I guess this is supported with ext-vectors, but I don't see any reason to support it for "regular" vectors.

[bruno]

Update after Akira's comment

[bruno]

Update again (now with the right patch) after Akira's comment

[ahatanak]

Thanks Bruno. I have a couple more questions.

lib/Sema/SemaExpr.cpp

8090

Sorry I wasn't clear, but I was asking whether you were planning to allow the following conversions for compound statements.

typedef short s4 __attribute__ ((vector_size(8)));
typedef int i2 __attribute__ ((vector_size(8)));
s4 a;
i2 b;
a = a + b; // clang accepts this.
a += b; // currently clang rejects this.

Also, I feel clang is inconsistent in warning about incompatible types. In the following example, it issues a warning for the first line, but is silent about the second line:

a = b + a; // incompatible vector types warning
a = a + b; // no warning

I don't think we have to fix everything in this patch, but just wanted to know what your thoughts were.

[bruno]

lib/Sema/SemaExpr.cpp

8090

You're right, the diagnostics are bad here, this patch adds some FIXMEs so we can later work on it. A PR would be nice though (we have an internal track for that as well, see rdar://problem/28067874).

Given your example:

a = a + b; // clang accepts this. 
a += b; // currently clang rejects this.

IMO, clang should reject a = a + b too if not in OpenCL mode, which means whenever (a) a and b have same size but different num elt and (b) a and b are generic vectors (non ext-vectors). However, It looks like we have tests that rely on this behavior, we should probably find out why first and clean it up.

I also think we should support splatting when one of the operands is a scalar and the other a non ext-vector (as of now we currently only do it for OpenCL). This is basically what GCC supports https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html.

[bruno]

@rnk, do you have any concerns about this patch?

[rnk]

lgtm

lib/Sema/SemaExpr.cpp

8084

typo on biscast

8090

I agree. I think the way forward is to:

• Allow conversions between <1 x T> and scalar T without enabling lax vector conversions. This seems pretty unobjectionable.
• Disable lax vector conversions by default, and evaluate whether they can be completely removed.

[bruno]

Thanks Reid & Akira,

Committed r282968

lib/Sema/SemaExpr.cpp

if (!IsCompAssign &&
         RHSVecType->getElementType()->isIntegerType()) {
       LHS = ImpCastExprToType(LHS.get(), RHSType, CK_BitCast);
       return RHSType;
     }
   }
 
   // If there's an ext-vector type and a scalar, try to convert the scalar to
   // the vector element type and splat.
+  // FIXME: this should also work for regular vector types as supported in GCC.
   if (!RHSVecType && isa<ExtVectorType>(LHSVecType)) {
     if (!tryVectorConvertAndSplat(*this, &RHS, RHSType,
                                   LHSVecType->getElementType(), LHSType))
       return LHSType;
   }
   if (!LHSVecType && isa<ExtVectorType>(RHSVecType)) {
     if (!tryVectorConvertAndSplat(*this, (IsCompAssign ? nullptr : &LHS),
                                   LHSType, RHSVecType->getElementType(),
                                   RHSType))
       return RHSType;
   }
 
-  // If we're allowing lax vector conversions, only the total (data) size needs
-  // to be the same. If one of the types is scalar, the result is always the
-  // vector type. Don't allow this if the scalar operand is an lvalue.
+  // FIXME: The code below also handles convertion between vectors and
+  // non-scalars, we should break this down into fine grained specific checks
+  // and emit proper diagnostics.
   QualType VecType = LHSVecType ? LHSType : RHSType;
-  QualType ScalarType = LHSVecType ? RHSType : LHSType;
-  ExprResult *ScalarExpr = LHSVecType ? &RHS : &LHS;
-  if (isLaxVectorConversion(ScalarType, VecType) &&
-      !ScalarExpr->get()->isLValue()) {
-    *ScalarExpr = ImpCastExprToType(ScalarExpr->get(), VecType, CK_BitCast);
+  const VectorType *VT = LHSVecType ? LHSVecType : RHSVecType;
+  QualType OtherType = LHSVecType ? RHSType : LHSType;
+  ExprResult *OtherExpr = LHSVecType ? &RHS : &LHS;
+  if (isLaxVectorConversion(OtherType, VecType)) {
+    // If we're allowing lax vector conversions, only the total (data) size
+    // needs to be the same. For non compound assignment, if one of the types is
+    // scalar, the result is always the vector type.
+    if (!IsCompAssign) {
+      *OtherExpr = ImpCastExprToType(OtherExpr->get(), VecType, CK_BitCast);
+      return VecType;
+    // In a compound assignment, lhs += rhs, 'lhs' is a lvalue src, forbidding
+    // any implicit cast. Here, the 'rhs' should be implicit casted to 'lhs'
+    // type. Note that this is already done by non-compound assignments in
+    // CheckAssignmentConstraints. If it's a scalar type, only biscast for
+    // <1 x T> -> T. The result is also a vector type.
+    } else if (OtherType->isExtVectorType() ||
+               (OtherType->isScalarType() && VT->getNumElements() == 1)) {
+      ExprResult *RHSExpr = &RHS;
+      *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast);
       return VecType;
     }
+  }
 
   // Okay, the expression is invalid.
 
   // If there's a non-vector, non-real operand, diagnose that.
   if ((!RHSVecType && !RHSType->isRealType()) ||
       (!LHSVecType && !LHSType->isRealType())) {
     Diag(Loc, diag::err_typecheck_vector_not_convertable_non_scalar)
       << LHSType << RHSType

test/Sema/vector-cast.c

 typedef __attribute__((vector_size(8))) double float64x1_t;
 typedef __attribute__((vector_size(16))) double float64x2_t;
 float64x1_t vget_low_f64(float64x2_t __p0);
 
 void f4() {
   float2 f2;
   double d, a, b, c;
   float64x2_t v = {0.0, 1.0};
-  f2 += d;
+  // FIXME: These diagnostics are inaccurate: should complain that 'double' to vector 'float2' involves truncation
+  f2 += d; // expected-error {{cannot convert between vector values of different size ('float2' (vector of 2 'float' values) and 'double')}}
+  d += f2; // expected-error {{cannot convert between vector values of different size}}
   a = 3.0 + vget_low_f64(v);
   b = vget_low_f64(v) + 3.0;
   c = vget_low_f64(v);
-  // LAX conversions within compound assignments are not supported.
-  // FIXME: This diagnostic is inaccurate.
-  d += f2; // expected-error {{cannot convert between vector values of different size}}
-  c -= vget_low_f64(v); // expected-error {{cannot convert between vector values of different size}}
+  c -= vget_low_f64(v);
   // LAX conversions between scalar and vector types require same size and one element sized vectors.
   d = f2; // expected-error {{assigning to 'double' from incompatible type 'float2'}}
   d = d + f2; // expected-error {{assigning to 'double' from incompatible type 'float2'}}
 }
 
 // rdar://15931426
 // Don't permit a lax conversion to and from a pointer type.
 typedef short short_sizeof_pointer __attribute__((vector_size(sizeof(void*))));
 void f5() {
   short_sizeof_pointer v;
   void *ptr;
   v = ptr; // expected-error-re {{assigning to 'short_sizeof_pointer' (vector of {{[0-9]+}} 'short' values) from incompatible type 'void *'}}
   ptr = v; // expected-error {{assigning to 'void *' from incompatible type 'short_sizeof_pointer'}}
 }