Vector shifting patch

Here is a version with improved build_vector_from val.

ChangeLog:

2010-10-25  Artjoms Sinkarovs <artyom.shinakroff@gmail.com>
  Andrew Pinski <pinskia@gmail.com>

  gcc/
  * tree.h (build_vector_from_val): Declare.
  * tree.c (build_vector_from_val): New function.
  * c-typeck.c (build_binary_op): Handle vector shifting.

  gcc/testsuite/
  * gcc.c-torture/execute/vector-shift.c: Likewise.
  * gcc.c-torture/execute/vector-shift1.c: New testcase.
  * gcc.c-torture/execute/vector-shift2.c: New testcase.
  * gcc.dg/vector-shift.c: New testcase.
  * gcc.dg/vector-shift1.c: New testcase.
  * gcc.dg/vector-shift2.c: New testcase.
  * gcc.dg/vector-shift3.c: New testcase.
  * gcc.dg/vector-shift4.c: New testcase.

bootstrapped and tested on x86_64_unknown-linux

OK?


On Mon, Oct 25, 2010 at 4:27 PM, Nathan Froyd <froydnj@codesourcery.com> wrote:
> On Mon, Oct 25, 2010 at 04:21:47PM +0100, Artem Shinkarov wrote:
>> Does it look better now:
>>
>> tree
>> build_vector_from_val (const tree sc, const tree vectype)
>> {
>>   int i, nunits = TYPE_VECTOR_SUBPARTS (vectype);
>>   VEC(constructor_elt, gc) *v;
>>
>>   if (sc == error_mark_node)
>>     return sc;
>>
>>   gcc_assert (TREE_TYPE (sc) == TREE_TYPE (vectype));
>>
>>   v = VEC_alloc (constructor_elt, gc, nunits);
>>   for (i = 0; i < nunits; ++i)
>>     CONSTRUCTOR_APPEND_ELT (v, size_int (i), sc);
>                                 ^^^^^^^^^^^^
>
> You can just pass NULL_TREE here and the Right Thing will happen.
>
> I think the logic looks fine.  For consistency's sake, it would probably
> be good to have the argument order reversed, i.e.:
>
> tree
> build_vector_from_val (const_tree vectype, const_tree sc)
>
> as that would more closely match the existing build_vector* functions.
>
> Please also note that you want const_tree instead of 'const tree' (note
> underscore).  If there are other instances of that in your patch, you'll
> want to fix those too.
>
> -Nathan
>

On Mon, 25 Oct 2010, Artem Shinkarov wrote:

> Index: gcc/doc/extend.texi
> ===================================================================
> --- gcc/doc/extend.texi	(revision 165913)
> +++ gcc/doc/extend.texi	(working copy)
> @@ -6309,6 +6309,23 @@ v4si a, b, c;
>  c = a + b;
>  @end smallexample
>  
> +In C it is possible to use shifting operators @code{<<, >>} on integer-type
> +vectors. The operation is defined as following: @code{@{a0, a1, ..., an@} >> @{b0, b1,
> +..., bn@} == @{a0 >> b0, a1 >> b1, ..., an >> bn@}}@.  Additionally one of the

The "..." here should be @dots{} since it is not the literal C token 
"...".

> +operands can be a scalar integer in which case the scalar is converted to the
> +type used by the vector operand and each element of this new vector is the
> +scalar's value.  Consider the following code.

Most of my comments relate to the scalar case which seems more potentially 
problematic than the case where both operands are vectors.  But in the 
case where both operands are vectors I note you say nothing about 
permitted types of the two operands.  Are they required to be exactly the 
same?  Or can they differ in signedness?  In the width of elements of the 
vector?

In normal C without vectors, the types of the two operands of a shift are 
independent.  This would tend to suggest that this should be the case for 
vectors - they must have the same number of elements, but why constrain 
things further?  I didn't see any such constraint in the OpenCL 
specification, although I note that OpenCL doesn't permit scalar << vector 
or scalar >> vector.

Say one operand is a scalar.  There doesn't appear to be much coverage of 
this case in the testcases added.  If there is a scalar shift amount, 
where you say:

> +  /* For 'vector <shift> scalar' or 'scalar <shift> vector', we convert 
> +     a scalar to a vector. Truncating the shift amount is ok.  */

then indeed this truncation is OK, because the only times the value will 
be affected are cases where the shift amount would be out of range and 
there would be undefined behavior (in C).  But if you are shifting a 
scalar by a vector, say 0x12345678 << (vector unsigned char){1,1,1,1}, 
then converting the scalar to the type of the vector elements seems 
inappropriate and risky.  It would at least require the use of 
convert_and_check so appropriate warnings can be given, but it would seem 
better to disallow this case, as in OpenCL.  If allowed then you'd want 
the result to be a vector whose element type is the type of the LHS of the 
shift - but C promotions would prevent the LHS from having types such as 
char and short.

> +  if ((code0 == VECTOR_TYPE || code1 == VECTOR_TYPE)
> +      && (code0 != code1))
> +    {
> +      switch (code)
> +        {
> +          case RSHIFT_EXPR:
> +          case LSHIFT_EXPR:
> +            if (code0 == INTEGER_TYPE)
> +              {
> +                tree sc = save_expr (op0);
> +                sc = convert (TREE_TYPE (type1), sc);
> +                op0 = build_vector_from_val (type1, sc);
> +                orig_type0 = type0 = TREE_TYPE (op0);
> +                code0 = TREE_CODE (type0);
> +              }
> +            break;

If you do disallow such shifts this code could go away - but why are you 
allowing in this particular place only for scalars shifted by a vector 
amount and not for vectors shifted by a scalar amount?  Where is the code 
handling vectors shifted by scalars?

On Tue, Oct 26, 2010 at 4:15 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Mon, 25 Oct 2010, Artem Shinkarov wrote:
>
>> Index: gcc/doc/extend.texi
>> ===================================================================
>> --- gcc/doc/extend.texi       (revision 165913)
>> +++ gcc/doc/extend.texi       (working copy)
>> @@ -6309,6 +6309,23 @@ v4si a, b, c;
>>  c = a + b;
>>  @end smallexample
>>
>> +In C it is possible to use shifting operators @code{<<, >>} on integer-type
>> +vectors. The operation is defined as following: @code{@{a0, a1, ..., an@} >> @{b0, b1,
>> +..., bn@} == @{a0 >> b0, a1 >> b1, ..., an >> bn@}}@.  Additionally one of the
>
> The "..." here should be @dots{} since it is not the literal C token
> "...".

Thanks, fixed.

>
>> +operands can be a scalar integer in which case the scalar is converted to the
>> +type used by the vector operand and each element of this new vector is the
>> +scalar's value.  Consider the following code.
>
> Most of my comments relate to the scalar case which seems more potentially
> problematic than the case where both operands are vectors.  But in the
> case where both operands are vectors I note you say nothing about
> permitted types of the two operands.  Are they required to be exactly the
> same?  Or can they differ in signedness?  In the width of elements of the
> vector?

In case when both operands are vectors my patch requires that they are
both integer-typed and that the number of elements is the same. On the
other hand, the existing code c-typeck.c:(build_binary_op):10048

  if (code0 == VECTOR_TYPE && code1 == VECTOR_TYPE
      && (!tree_int_cst_equal (TYPE_SIZE (type0), TYPE_SIZE (type1))
	  || !same_scalar_type_ignoring_signedness (TREE_TYPE (type0),
						    TREE_TYPE (type1))))
    {
      binary_op_error (location, code, type0, type1);
      return error_mark_node;
    }

requires that the base type of the vector has the same width.

>
> In normal C without vectors, the types of the two operands of a shift are
> independent.  This would tend to suggest that this should be the case for
> vectors - they must have the same number of elements, but why constrain
> things further?  I didn't see any such constraint in the OpenCL
> specification, although I note that OpenCL doesn't permit scalar << vector
> or scalar >> vector.

Again, I agree, but then we should do the same stuff for operations +,
-, *, /, ...
because we can have an expression in C like: <int> + <char> * <short>
and the type inference would do the job.
On the other hand if we allow that, then most likely most of our
vector operations will be expanded and would not reach the hardware.
And I think that one of the purpose of these vector extensions is to
build an interface for CPU SIMD instructions. So this is a sort of
political decision we want to make.

My thinking here was that we leave the patch as is, and if later we
will decide that we want to support vector(8) char <op> vector(8)
short, we would just remove the constraint and my patch would still
work.

>
> Say one operand is a scalar.  There doesn't appear to be much coverage of
> this case in the testcases added.  If there is a scalar shift amount,
> where you say:
>
>> +  /* For 'vector <shift> scalar' or 'scalar <shift> vector', we convert
>> +     a scalar to a vector. Truncating the shift amount is ok.  */
>
> then indeed this truncation is OK, because the only times the value will
> be affected are cases where the shift amount would be out of range and
> there would be undefined behavior (in C).  But if you are shifting a
> scalar by a vector, say 0x12345678 << (vector unsigned char){1,1,1,1},
> then converting the scalar to the type of the vector elements seems
> inappropriate and risky.  It would at least require the use of
> convert_and_check so appropriate warnings can be given, but it would seem
> better to disallow this case, as in OpenCL.  If allowed then you'd want
> the result to be a vector whose element type is the type of the LHS of the
> shift - but C promotions would prevent the LHS from having types such as
> char and short.

OpenCL allows scalar <op> vector for most of the arithmetic
operations, so I thought that it would be nice to have this for shifts
as well, considering the fact that vector shift where the second
operator is a scalar is ok.

I don't see how the conversion of the expression 0x12345678 <<
{1,1,1,1} in compile time is different from the conversion of the
expression:
vector int x = (vector int){0x12345678, 0x123456778, ...} << {1,1,1,1}
which is allowed. Ok, in that case we will get warnings if -Woferflow
is on.

But in the case:
long long x;
vecor(8) short v0 = {...};
v0 = (vector(8) short){x,x,...} << v0;
we would not get any warnings and we will have the same behaviour as in my case.
And of course I can use convert_and_check instead of just convert.

>
>> +  if ((code0 == VECTOR_TYPE || code1 == VECTOR_TYPE)
>> +      && (code0 != code1))
>> +    {
>> +      switch (code)
>> +        {
>> +          case RSHIFT_EXPR:
>> +          case LSHIFT_EXPR:
>> +            if (code0 == INTEGER_TYPE)
>> +              {
>> +                tree sc = save_expr (op0);
>> +                sc = convert (TREE_TYPE (type1), sc);
>> +                op0 = build_vector_from_val (type1, sc);
>> +                orig_type0 = type0 = TREE_TYPE (op0);
>> +                code0 = TREE_CODE (type0);
>> +              }
>> +            break;
>
> If you do disallow such shifts this code could go away - but why are you
> allowing in this particular place only for scalars shifted by a vector
> amount and not for vectors shifted by a scalar amount?  Where is the code
> handling vectors shifted by scalars?

The code is in the case  {RL}SHIFT_EXPR: branches. The thing is that
middleend happily accepts the expression vector >> scalar and vector
<< scalar. That is why I don't have to do anything there, just to
build that expression.

>
> --
> Joseph S. Myers
> joseph@codesourcery.com
>

So who is taking the political decision about the binary operation on
vectors of different width?

On Mon, Oct 25, 2010 at 06:15:40PM +0100, Artem Shinkarov wrote:
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c	(revision 165913)
> +++ gcc/tree.c	(working copy)
> @@ -1366,6 +1366,28 @@ build_vector_from_ctor (tree type, VEC(c
>    return build_vector (type, nreverse (list));
>  }
>  
> +/* Build a vector of type VECTYPE where all the elements are SCs.  */
> +tree
> +build_vector_from_val (tree vectype, tree sc) 
> +{
> +  int i, nunits = TYPE_VECTOR_SUBPARTS (vectype);
> +  VEC(constructor_elt, gc) *v = NULL;
> +
> +  if (sc == error_mark_node)
> +    return sc;
> +
> +  gcc_assert (TREE_TYPE (sc) == TREE_TYPE (vectype));
> +
> +  v = VEC_alloc (constructor_elt, gc, nunits);
> +  for (i = 0; i < nunits; ++i)
> +    CONSTRUCTOR_APPEND_ELT (v, size_int (i), sc);
                                  ^^^^^^^^^^^^

Passing this is not necessary and is wrong when you would need to call
build_constructor.  Please pass NULL_TREE here.

-Nathan

On Wed, 27 Oct 2010, Artem Shinkarov wrote:

> > Most of my comments relate to the scalar case which seems more potentially
> > problematic than the case where both operands are vectors.  But in the
> > case where both operands are vectors I note you say nothing about
> > permitted types of the two operands.  Are they required to be exactly the
> > same?  Or can they differ in signedness?  In the width of elements of the
> > vector?
> 
> In case when both operands are vectors my patch requires that they are
> both integer-typed and that the number of elements is the same. On the
> other hand, the existing code c-typeck.c:(build_binary_op):10048
> 
>   if (code0 == VECTOR_TYPE && code1 == VECTOR_TYPE
>       && (!tree_int_cst_equal (TYPE_SIZE (type0), TYPE_SIZE (type1))
> 	  || !same_scalar_type_ignoring_signedness (TREE_TYPE (type0),
> 						    TREE_TYPE (type1))))
>     {
>       binary_op_error (location, code, type0, type1);
>       return error_mark_node;
>     }
> 
> requires that the base type of the vector has the same width.

But shouldn't the documentation make this explicit for shifts?

There's a general statement in the documentation already that both length 
and signedness must match - but you're describing shifts specially so it 
may not be clear what general statements apply to them.  And why is your 
paragraph about shifts going in between an example for addition, and a 
statement that "Subtraction, multiplication, division, and the logical 
operations operate in a similar manner." - that is, in a similar manner to 
addition?  The effect would seem to be to change the meaning of "similar" 
to refer to shifts rather than addition.  That doesn't seem to be a good 
idea.  Work out what the documentation should look like for the extension 
as a coherent whole, and it might be clearer what if any new statement 
about types is needed.

> > In normal C without vectors, the types of the two operands of a shift are
> > independent.  This would tend to suggest that this should be the case for
> > vectors - they must have the same number of elements, but why constrain
> > things further?  I didn't see any such constraint in the OpenCL
> > specification, although I note that OpenCL doesn't permit scalar << vector
> > or scalar >> vector.
> 
> Again, I agree, but then we should do the same stuff for operations +,
> -, *, /, ...
> because we can have an expression in C like: <int> + <char> * <short>
> and the type inference would do the job.

In C, the integer promotions apply to operands of both + and <<.  For 
vector operations they are deliberately excluded - you can operate on 
vector char, but not directly on char.

However, in C the *usual arithmetic conversions* (a superset of the 
integer promotions) apply for + but not <<; there is no requirement to 
form a common type as there is for arithmetic operations.  So the general 
vector rules appear to be being expanded to say:

* no integer promotions

* no usual arithmetic conversions

* must have a common type for vectors (without such promotions / 
conversions) even for shifts when ISO C doesn't require a common type for 
scalars

> OpenCL allows scalar <op> vector for most of the arithmetic
> operations, so I thought that it would be nice to have this for shifts
> as well, considering the fact that vector shift where the second
> operator is a scalar is ok.
> 
> I don't see how the conversion of the expression 0x12345678 <<
> {1,1,1,1} in compile time is different from the conversion of the
> expression:
> vector int x = (vector int){0x12345678, 0x123456778, ...} << {1,1,1,1}
> which is allowed. Ok, in that case we will get warnings if -Woferflow
> is on.

The point I had was when the shift amount was vector char - so if you 
convert the LHS to the type of the RHS then you have (vector char){0x78, 
0x78, 0x78, 0x78}.  Truncating a scalar like that seems a further step 
beyond the rules listed above, and it doesn't seem a particularly 
desirable step to me.