[PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno closed this revision. bruno added a comment. Thanks Reid & Akira, Committed r282968 https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24472: [Sema] Support lax conversions for compound assignments
rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. lgtm > SemaExpr.cpp:8084 > +// 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. typo on biscast > bruno wrote in 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. 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. https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno added a comment. @rnk, do you have any concerns about this patch? https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8090 @@ +8089,3 @@ + *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast); + return VecType; +} ahatanak wrote: > 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. 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. https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
ahatanak added a comment. Thanks Bruno. I have a couple more questions. Comment at: lib/Sema/SemaExpr.cpp:8090 @@ +8089,3 @@ + *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast); + return VecType; +} 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. https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno updated this revision to Diff 71870.
bruno added a comment.
Update again (now with the right patch) after Akira's comment
https://reviews.llvm.org/D24472
Files:
lib/Sema/SemaExpr.cpp
test/Sema/vector-cast.c
Index: test/Sema/vector-cast.c
===
--- test/Sema/vector-cast.c
+++ test/Sema/vector-cast.c
@@ -53,14 +53,13 @@
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'}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -8051,6 +8051,7 @@
// 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(LHSVecType)) {
if (!tryVectorConvertAndSplat(*this, &RHS, RHSType,
LHSVecType->getElementType(), LHSType))
@@ -8063,16 +8064,31 @@
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);
-return VecType;
+ 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.
Index: test/Sema/vector-cast.c
===
--- test/Sema/vector-cast.c
+++ test/Sema/vector-cast.c
@@ -53,14 +53,13 @@
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 ve
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno updated this revision to Diff 71869.
bruno marked an inline comment as done.
bruno added a comment.
Update after Akira's comment
https://reviews.llvm.org/D24472
Files:
lib/Sema/SemaExpr.cpp
test/Sema/vector-cast.c
Index: test/Sema/vector-cast.c
===
--- test/Sema/vector-cast.c
+++ test/Sema/vector-cast.c
@@ -53,14 +53,13 @@
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'}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -8051,6 +8051,7 @@
// 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(LHSVecType)) {
if (!tryVectorConvertAndSplat(*this, &RHS, RHSType,
LHSVecType->getElementType(), LHSType))
@@ -8063,16 +8064,31 @@
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);
-return VecType;
+ 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.
+} else if (OtherType->isExtVectorType() ||
+ (OtherType->isScalarType() && VT->getNumElements() == 1)) {
+ ExprResult *RHSExpr = &RHS;
+ *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast);
+ return LHSType;
+}
}
// Okay, the expression is invalid.
Index: test/Sema/vector-cast.c
===
--- test/Sema/vector-cast.c
+++ test/Sema/vector-cast.c
@@ -53,14 +53,13 @@
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 si
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno marked an inline comment as done. Comment at: lib/Sema/SemaExpr.cpp:8084 @@ +8083,3 @@ + *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast); + return LHSType; +} ahatanak wrote: > 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? 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. https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
ahatanak added inline comments. Comment at: lib/Sema/SemaExpr.cpp:8084 @@ +8083,3 @@ + *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast); + return LHSType; +} 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? https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno added a comment. Ping! https://reviews.llvm.org/D24472 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D24472: [Sema] Support lax conversions for compound assignments
bruno created this revision.
bruno added a reviewer: rnk.
bruno added subscribers: cfe-commits, ahatanak.
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.
https://reviews.llvm.org/D24472
Files:
lib/Sema/SemaExpr.cpp
test/Sema/vector-cast.c
Index: test/Sema/vector-cast.c
===
--- test/Sema/vector-cast.c
+++ test/Sema/vector-cast.c
@@ -53,14 +53,13 @@
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'}}
Index: lib/Sema/SemaExpr.cpp
===
--- lib/Sema/SemaExpr.cpp
+++ lib/Sema/SemaExpr.cpp
@@ -8045,6 +8045,7 @@
// 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(LHSVecType)) {
if (!tryVectorConvertAndSplat(*this, &RHS, RHSType,
LHSVecType->getElementType(), LHSType))
@@ -8057,16 +8058,31 @@
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);
-return VecType;
+ 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.
+} else if (OtherType->isExtVectorType() ||
+ (OtherType->isScalarType() && VT->getNumElements() == 1)) {
+ ExprResult *RHSExpr = &RHS;
+ *RHSExpr = ImpCastExprToType(RHSExpr->get(), LHSType, CK_BitCast);
+ return LHSType;
+}
}
// Okay, the expression is invalid.
Index: test/Sema/vector-cast.c
===
--- test/Sema/vector-cast.c
+++ test/Sema/vector-cast.c
@@ -53,14 +53,13 @@
float2 f2;
do
