This is an archive of the discontinued LLVM Phabricator instance.

[Sema] Support implicit scalar to vector conversions
ClosedPublic

Authored by sdardis on Oct 21 2016, 2:58 AM.

Details

Summary

This patch teaches clang to perform implicit scalar to vector conversions
when one of the operands of a binary vector expression is a scalar which
can be converted to the element type of the vector without truncation
following GCC's implementation.

If the (constant) scalar is can be casted safely, it is implicitly casted to the
vector elements type and splatted to produce a vector of the same type.

Contributions from: Petar Jovanovic

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 75401.Oct 21 2016, 2:58 AM
sdardis retitled this revision from to [Sema] Support implicit scalar to vector conversions.
sdardis updated this object.
sdardis added subscribers: rnk, bruno, ahatanak, cfe-commits.
sdardis updated this object.Oct 21 2016, 3:00 AM
sdardis edited edge metadata.
sdardis added a subscriber: petarj.
ahatanak added inline comments.Oct 21 2016, 9:28 AM
lib/Sema/SemaExpr.cpp
8044 ↗(On Diff #75401)

Is this the same as "!RHSVecType && LHSVecType" in this context?

ahatanak added inline comments.Oct 21 2016, 9:41 AM
lib/Sema/SemaExpr.cpp
8044 ↗(On Diff #75401)

Actually, LHS's type should be a vector here if RHS isn't a vector since CheckVectorOperands is called only when at least one operand is a vector.

sdardis updated this revision to Diff 75708.Oct 25 2016, 8:17 AM
sdardis marked an inline comment as done.

Extra testing for cases where the operand on the left of an operation is a vector.
Removed two spurious checks for vector types.

bruno added a comment.Oct 25 2016, 9:15 AM

Hi,

Nice, thanks for working on this!

lib/Sema/SemaExpr.cpp
8051 ↗(On Diff #75708)

tryVectorConvertAndSplat does more than plain scalar splat; it supports a more general type of CK_IntegralCast, see the comment on one of your changes to the tests below.

I suggest that instead of reusing this function, you should create another one that only handles the cases we actually want to support for non-ext vectors (i.e. for GCC compat).

test/Sema/vector-scalar-implict-conv.c
2 ↗(On Diff #75708)

Can you rename this to vector-gcc-compat.c? It would also be nice to split functionality being tested within their own function, e.g.: arithmetic, logic, vector comparisons.

bruno added inline comments.Oct 25 2016, 9:15 AM
test/Sema/vector-cast.c
57 ↗(On Diff #75708)

This is not right. The fact that we don't have the appropriate diagnostics here doesn't mean we should accept this. For instance, this is what we get with GCC:

error: conversion of scalar 'double' to vector 'float2 {aka __vector(2) float}' involves truncation

bruno added a reviewer: bruno.Oct 25 2016, 9:16 AM
sdardis updated this revision to Diff 76385.Oct 31 2016, 5:00 AM
sdardis updated this object.
sdardis edited edge metadata.

Split out a variant of tryVectorConvertAndSplat called tryGCCVectorConvertAndSplat. This variant checks the types more strictly than tryVectorConvertAndSplat and handles implicit conversion of constants which can be safely demoted to a smaller type.
Added more testing to vector-gcc-compat.c

bruno added inline comments.Nov 17 2016, 6:10 PM
lib/Sema/SemaExpr.cpp
7978 ↗(On Diff #76385)

Remove this empty line.

7991 ↗(On Diff #76385)

This can be simplified by checking isArithmeticType() for each instead.

8005 ↗(On Diff #76385)

You already checked this condition in the if above, this will never trigger.

8043 ↗(On Diff #76385)

I don't see how VectorEltTy != ScalarTy is possible here.

8064 ↗(On Diff #76385)

I don't see why it's necessary to check for all specific cases where the scalar is a constant. For all the others scenarios it should be enough to get the right answer via getIntegerTypeOrder or getFloatTypeOrder. For this is specific condition, the else part for the CstScalar below should also handle the constant case, right?

8267 ↗(On Diff #76385)

This change seems orthogonal to this patch. Can you make it a separated patch with the changes from test/Sema/vector-cast.c?

test/Sema/vector-gcc-compat.c
2 ↗(On Diff #76385)

These are really nice tests. Some cosmetic cleanups: can you run it through clang-format and also remove (a) newlines between comments and codes, (b) places with double newlines?

sdardis updated this revision to Diff 78531.Nov 18 2016, 8:12 AM
sdardis edited edge metadata.
sdardis marked 4 inline comments as done.

Addressed review comments.

sdardis added inline comments.Nov 18 2016, 8:14 AM
lib/Sema/SemaExpr.cpp
8064 ↗(On Diff #76385)

If we have a scalar constant, it is necessary to examine it's actual value to see if it can be casted without truncation. The scalar constant's type is somewhat irrelevant. Consider:

v4f32 f(v4f32 a) {
  return a + (double)1.0;
}

In this case examining the order only works if the vector's order is greater than or equal to the scalar constant's order. Instead, if we ask whether the scalar constant can be represented as a constant scalar of the vector's element type, then we know if we can convert without the loss of precision.

For integers, the case is a little more contrived due to native integer width, but the question is the same. Can a scalar constant X be represented as a given floating point type. Consider:

v4f32 f(v4f32 a) {
  return a + (signed int)1;
 }

This would rejected for platforms where a signed integer's width is greater than the precision of float if we examined it based purely on types and their sizes. Instead, if we convert the integer to the float point's type with APFloat and convert back to see if we have the same value, we can determine if the scalar constant can be safely converted without the loss of precision.

I based the logic examining the value of the constant scalar based on GCC's behaviour, which implicitly converts scalars regardless of their type if they can be represented in the same type of the vector's element type without the loss of precision. If the scalar cannot be evaluated to a constant at compile time, then the size in bits for the scalar's type determines if it can be converted safely.

8267 ↗(On Diff #76385)

This change is a necessary part of this patch and it can't be split out in sensible fashion.

For example, line 329 of test/Sema/zvector.c adds a scalar signed character to a vector of signed characters. With just this change we would report "cannot convert between scalar type 'signed char' and vector type '__vector signed char' (vector of 16 'signed char' values) as implicit conversion would cause truncation".

This is heavily misleading for C and C++ code as we don't perform implicit conversions and signed char could be implicitly converted to a vector of signed char without the loss of precision.

To make this diagnostic precise without performing implicit conversions requires determining cases where implicit conversion would cause truncation and reporting that failure reason, or defaulting to the generic diagnostic.

Keeping this change as part of this patch is cleaner and simpler as it covers both implicit conversions and more precise diagnostics.

bruno added inline comments.Nov 28 2016, 2:02 PM
lib/Sema/SemaExpr.cpp
8064 ↗(On Diff #76385)

Right. Can you split the scalarTy <-> vectorEltTy checking logic into separate functions; one for cst-scalar-int-to-vec-elt-int and another for scalar-int-to-vec-elt-float?

8267 ↗(On Diff #76385)

Can you double check whether we should support these GCC semantics for getLangOpts().ZVector? If not, then this should be introduced in a separate patch/commit.

8020 ↗(On Diff #78531)

Please also early return !CstScalar && Order < 0 like you did below.

8064 ↗(On Diff #78531)

ignored => Ignored

8171 ↗(On Diff #78531)

It's not clear to me whether clang should support the GCC semantics when in getLangOpts().AltiVec || getLangOpts().ZVector, do you?

You can remove the curly braces for the if and else here.

8182 ↗(On Diff #78531)

Also remove braces here.

sdardis updated this revision to Diff 81755.Dec 16 2016, 7:36 AM
sdardis edited edge metadata.
sdardis marked an inline comment as done.

Addressed review comments, fixed assertion issue with expressions like scalar -= vector.

sdardis added inline comments.Dec 16 2016, 7:47 AM
lib/Sema/SemaExpr.cpp
8267 ↗(On Diff #76385)

See my comment above.

8171 ↗(On Diff #78531)

We should, GCC performs scalar to vector conversions regardless of what cpu features are available. If the target machine doesn't have vector support, GCC silently scalarizes the operation.

https://gcc.gnu.org/onlinedocs/gcc-6.2.0/gcc/Vector-Extensions.html#Vector-Extensions

8051 ↗(On Diff #75708)

(ignore this, phabricator's web interface is acting up)

bruno edited edge metadata.Mar 6 2017, 2:17 PM

Hey, really sorry for the delay here.

lib/Sema/SemaExpr.cpp
8007 ↗(On Diff #81755)

This doesn't look clang-formated.

8034 ↗(On Diff #81755)

Instead of the else here, you can:

....
   return (IntSigned != OtherIntSigned &&
       NumBits > S.Context.getIntWidth(OtherIntTy))
}

return (Order < 0);
8092 ↗(On Diff #81755)

Can you please add an assertion to the beginning of this function to guarantee that the vector is never a ext-vector type? I wanna make sure we don't accidentally use this in the future for ext-vectors.

test/Sema/vector-gcc-compat.c
61 ↗(On Diff #81755)

Can you double check where 'long attribute((ext_vector_type(2)))' comes from?

We have regressed in the past year in the way ext-vector interacts with non-ext-vectors, and I don't wanna make it worse until we actually have time to fix that; there's a lot of code out there relying on bitcasts between ext-vectors and non-ext-vectors to bridge between intrinsics headers and ext-vector code.

sdardis marked 3 inline comments as done.Mar 7 2017, 4:31 AM

Thanks for getting back to this. I've traced the appearance of the ext_vector type to a piece of code that only produces ext-vector types for comparisons. I'm presuming that's wrong when clang is producing vectors implicitly in the non-OpenCL case. I'll update the diff once I've changed that.

test/Sema/vector-gcc-compat.c
61 ↗(On Diff #81755)

Sema::CheckVectorCompareOperands calls Sema::GetSignedVectorType which only returns ext-vector types. I presume that is now incorrect if we're producing vectors from literal scalars in the non OpenCL case.

sdardis updated this revision to Diff 91716.Mar 14 2017, 7:19 AM

Addressed review comments, add C++ specific test.

bruno added inline comments.Mar 27 2017, 11:14 AM
test/Sema/vector-gcc-compat.c
61 ↗(On Diff #81755)

Nice catch, can you please submit these specific changes as a separated patch and mark it as a prerequisite of this one?

sdardis updated this revision to Diff 94070.Apr 4 2017, 6:48 AM

Factored out the changes from D31337.

bruno added inline comments.Apr 4 2017, 10:15 AM
lib/Sema/SemaExpr.cpp
8032 ↗(On Diff #94070)

Double checking here: are there tests for the InvalidOperands case above?

8163 ↗(On Diff #94070)

Spalt -> Splat

8188 ↗(On Diff #94070)

Is this something that GCC is going to support at some point? Regardless of GCC, do we want this behavior?

10019 ↗(On Diff #94070)

Why !getLangOpts().CPlusPlus is a requirement here?

test/Sema/vector-g++-compat.cpp
1 ↗(On Diff #94070)

Can you rename this file to vector-gcc-compat.cpp instead?

155 ↗(On Diff #94070)

Remove all these extra new lines after the function signature here, in the functions below and in the other added test file for c++.

sdardis marked an inline comment as done.Apr 5 2017, 6:43 AM

Thanks for sticking with this.

I've held off updating the diff until D31667 (not D31337 as previously posted) is finished. Responses inlined.

lib/Sema/SemaExpr.cpp
8188 ↗(On Diff #94070)

Is this something that GCC is going to support at some point?

I've reread GCC's source for conversions of scalar to vectors types and it appears that GCC does attempt to convert constant real expressions to integers but it appears to only work for C++. It's a little odd.

Regardless of GCC, do we want this behavior?

Given my oversight of the support of implicit safe conversions of floating point constants to integer types, I believe we should do this in general. I'll rework the comment to be more accurate and add the necessary conversion checks.

10019 ↗(On Diff #94070)

GCC only supports the logical operators &&, ||, ! when compiling C++.

It's noted near the bottom half of this page: https://gcc.gnu.org/onlinedocs/gcc/Vector-Extensions.html

test/Sema/vector-g++-compat.cpp
155 ↗(On Diff #94070)

Will do.

bruno added inline comments.Apr 5 2017, 10:07 AM
lib/Sema/SemaExpr.cpp
8188 ↗(On Diff #94070)

Ok, improving the FIXME sounds good to me. We can add this extra functionality in a future patch.

10019 ↗(On Diff #94070)

Ok. It would be nice to find out if there's any actual good reason for it, otherwise we might as well support it for non C++. But no need to block on that; just make sure to annotate these places with FIXMEs so we can work on it in the future.

ahatanak added inline comments.Apr 5 2017, 6:22 PM
lib/Sema/SemaExpr.cpp
8024 ↗(On Diff #94070)

I think "!=" is easier to understand than "^" here.

8305 ↗(On Diff #94070)

"a vector"

sdardis updated this revision to Diff 94434.Apr 6 2017, 1:55 PM
sdardis marked 7 inline comments as done.

Addressed review comments.

I've changed InvalidVectorOperands() to not use InvalidOperands() after updating some tests. InvalidOperands() was receiving expressions with implicit casts, leading to misleading error messages.

bruno added inline comments.Apr 7 2017, 1:08 PM
lib/Sema/SemaExpr.cpp
10024 ↗(On Diff #94434)

Please mention in the comment the list of logical ops that this applies to: "!, &&, ||"

test/Sema/vector-gcc-compat.c
101 ↗(On Diff #94434)

Is this because of && and others only working in C++? If so, we need a better error message here, along the lines of "logical expression with vector only support in C++". If later on we decide to support it in non-C++, we then get rid of the warning.

rnk added a subscriber: rsmith.Apr 12 2017, 10:44 AM
sdardis updated this revision to Diff 95109.Apr 13 2017, 5:21 AM
sdardis marked 2 inline comments as done.

Addressed review comments. I've highlighted the relevant places with FIXME:s where we don't support GCC vector operations.

Realised I've some comments to submit.

lib/Sema/SemaExpr.cpp
8032 ↗(On Diff #94070)

Yes, this case is covered in the new diff in test/Sema/vector-ops.c and in test/Sema/vector-gcc-compat.c .

10024 ↗(On Diff #94434)

I've updated the relevant Check* functions with FIXME:s noting that they should handle vector types for compatibility with GCC.

test/Sema/vector-gcc-compat.c
101 ↗(On Diff #94434)

Yes, these operators are oddly only available in C++.

I've changed the error message to "logical expression with vector type[s] '<typedef'd name>' (vector of <N> '<element type>' values) [and '<other type>'] is only supported in C++".

sdardis updated this revision to Diff 96533.Apr 25 2017, 5:23 AM

Rebase + ping.

bruno accepted this revision.Apr 26 2017, 7:06 PM

Thanks for your patience! LGTM with a minor comment below.

test/Sema/vector-gcc-compat.c
101 ↗(On Diff #94434)

Great, can you also mention the type of the other hand side when it's not a vector?

This revision is now accepted and ready to land.Apr 26 2017, 7:06 PM
sdardis updated this revision to Diff 98783.May 12 2017, 9:28 AM
sdardis marked an inline comment as done.
sdardis edited the summary of this revision. (Show Details)

Updated error message for logical operations where one operand is a vector and the other isn't.

Updated summary.

I'll commit this shortly.

This revision was automatically updated to reflect the committed changes.

Thanks for the review.