diff mbox

Vector shifting patch

Message ID AANLkTikYQLr3vCKMJUFnyVS25_dG5pBJsdrbEFYZX_Sp@mail.gmail.com
State New
Headers show

Commit Message

Artem Shinkarov Oct. 27, 2010, 3:15 p.m. UTC 
On Wed, Oct 27, 2010 at 1:21 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> 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.

Oh yes, of course you are right, this paragraph must be one paragraph
lower. Now it looks like this:


>
>> > 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.
>

Ok, I do understand your concern, but my following patch
(http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01075.html) implements
scalar <op> vector and vector <op> scalar for op in {+, -, /, ...}. In
that patch for integer types I use the function int_fits_type_p before
I allow scalar to vector conversion. And it would be strange to allow
scalar <op> vector everywhere except shifting operators.

Would it be ok, if I'll use the same int_fits_type_p checking if scalr
fits in the vector type and if not then I'll throw an error. And I'll
add this statement in the documentation.

What do you think about it?



Artem.

Comments

Joseph Myers Oct. 27, 2010, 4:50 p.m. UTC | #1 
On Wed, 27 Oct 2010, Artem Shinkarov wrote:

> Ok, I do understand your concern, but my following patch
> (http://gcc.gnu.org/ml/gcc-patches/2010-08/msg01075.html) implements
> scalar <op> vector and vector <op> scalar for op in {+, -, /, ...}. In
> that patch for integer types I use the function int_fits_type_p before
> I allow scalar to vector conversion. And it would be strange to allow
> scalar <op> vector everywhere except shifting operators.
> 
> Would it be ok, if I'll use the same int_fits_type_p checking if scalr
> fits in the vector type and if not then I'll throw an error. And I'll
> add this statement in the documentation.

That seems safer - but if other operations between scalars and vectors 
aren't supported until a subsequent patch, that clearly indicates that you 
should not be including mixed scalar/vector shifts in this patch - you 
should make this patch only accept vector/vector shifts.  Mixed shifts can 
then go in the patch adding other mixed operations, and be reviewed in the 
context of the full documentation and set of testcases for that patch.
diff mbox

Patch

Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 165913)
+++ gcc/doc/extend.texi	(working copy)
@@ -6315,6 +6315,27 @@  minus or complement operators on a vecto
 elements are the negative or complemented values of the corresponding
 elements in the operand.

+In C it is possible to use shifting operators @code{<<, >>} on integer-type
+vectors. The operation is defined as following: @code{@{a0, a1, @dots{},
+an@} >> @{b0, b1, @dots{}, bn@} == @{a0 >> b0, a1 >> b1, @dots{}, an >>
+bn@}}@. Vector operands must have the same number of elements.
+Additionally one of the operands can be a scalar integer in which case the
+scalar is converted to the type used by the vector operand (with possible
+truncation) and each element of this new vector is the scalar's value.
+Consider the following code.
+
+@smallexample
+typedef int v4si __attribute__ ((vector_size (16)));
+
+v4si a, b, c;
+int i = 1;
+
+b = a >> 1;     /* b = a >> @{1,1,1,1@}; */
+c = 1 << a;     /* c = @{1,1,1,1@} << a; */
+@end smallexample
+
+
+
 In C vectors can be subscripted as if the vector were an array with
 the same number of elements and base type.  Out of bound accesses
 invoke undefined behavior at runtime.  Warnings for out of bound