diff mbox

Vector shifting patch

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

Commit Message

Artem Shinkarov June 15, 2010, 3:17 p.m. UTC 
This is a reworked Andrew Pinski's vector shifting patch. This is done
in terms of GSoC 2010 project.

The C part is separated from C++ part and is submitted as a separate patch.
OpenCL standard allows vector <op> scalar, scalar <op> vector
operations, for op = +,-,*,/,%. In that case scalar is converted to
the vector. Cannot see any reason why we would not want to support
these type of operations for shifting. So this is added to the patch
as well.

ChangeLog:

2010-06-14  Artem Shinkarov <artyom.shinakroff@gmail.com>
      Andrew Pinski <pinskia@gmail.com>

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

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

      doc/
      * extend.texi: Document vector shifting.

bootstrapped and tested on x86_64_unknown-linux

OK?

Comments

Andrew Pinski June 15, 2010, 4:33 p.m. UTC | #1 
On Tue, Jun 15, 2010 at 8:17 AM, Artem Shinkarov
<artyom.shinkaroff@gmail.com> wrote:
> This is a reworked Andrew Pinski's vector shifting patch. This is done
> in terms of GSoC 2010 project.
>
> The C part is separated from C++ part and is submitted as a separate patch.
> OpenCL standard allows vector <op> scalar, scalar <op> vector
> operations, for op = +,-,*,/,%. In that case scalar is converted to
> the vector. Cannot see any reason why we would not want to support
> these type of operations for shifting. So this is added to the patch
> as well.

You add a test to test for vector of floats that should be rejected.

+                sc = convert (TREE_TYPE (type1), sc);
+                op0 = build_vector_from_val (sc, type1);

Why don't you push the convert down into the build_vector_from_val.

+  gcc_assert (sc == error_mark_node
+              || TREE_TYPE (sc) == TREE_TYPE (vectype));

I think there could be a case where sc is error_mark_node which case
you should just return error_mark_node..
An example is:
#define vector __attribute__((vector_size(4*sizeof(int) )))
void g(void)
{
  vector int t;
  t <<= d; /* { dg-error "undeclared" }  */
}
--- CUT ---
How does your patch handle the case where <<= is used and the left
hand side is an integer type like:

#define vector __attribute__((vector_size(4*sizeof(int) )))
void g(void)
{
  vector int t;
  int d;
  d <<= t; /* { dg-error "" }  */
}

--- CUT ---
+typedef int v4si __attribute__ ((vector_size (16)));
I think that would be better if you do this:
+typedef int v4si __attribute__ ((vector_size (4*sizeof(int) )));

So that it is really v4 and I would name it v4int rather than saying
the internal GCC mode.

Some more from Joseph S. Myers' review of my patch which is why it was
not applied yet:

Do I understand correctly that it is also intended that the vectors must
have the same width and number of elements (but possibly different
signedness)?

If so, the testcases should cover this (errors for different width, errors
for different number of elements, allowing different signedness, errors if
either vector is floating point).

The tests should also verify that signed shift operations where the sign
bit is involved act as documented in implement-c.texi for individual
elements, and should test unsigned shift operations where the result
differs from that of signed shift (right shifting values with the top bit
set).  It would also be good to run tests for different base types and
vector lengths (after all, the conversion to scalar code could generate
things such as shifts on types smaller than int that wouldn't arise
directly in normal C code, so it may not be obvious that it's well-tested
whether such operations on char or short work correctly at present,
although certainly valid GIMPLE), and to test shift count vectors that do
not have all counts equal.

--- CUT ---

Thanks,
Andrew Pinski
Richard Biener June 16, 2010, 8:51 a.m. UTC | #2 
On Tue, Jun 15, 2010 at 6:33 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Tue, Jun 15, 2010 at 8:17 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> This is a reworked Andrew Pinski's vector shifting patch. This is done
>> in terms of GSoC 2010 project.
>>
>> The C part is separated from C++ part and is submitted as a separate patch.
>> OpenCL standard allows vector <op> scalar, scalar <op> vector
>> operations, for op = +,-,*,/,%. In that case scalar is converted to
>> the vector. Cannot see any reason why we would not want to support
>> these type of operations for shifting. So this is added to the patch
>> as well.
>
> You add a test to test for vector of floats that should be rejected.
>
> +                sc = convert (TREE_TYPE (type1), sc);
> +                op0 = build_vector_from_val (sc, type1);
>
> Why don't you push the convert down into the build_vector_from_val.

I asked him not to, convert shouldn't be called from generic
tree routines (I'd like to use build_vector_from_val from the
vectorizer as well).

> +  gcc_assert (sc == error_mark_node
> +              || TREE_TYPE (sc) == TREE_TYPE (vectype));
>
> I think there could be a case where sc is error_mark_node which case
> you should just return error_mark_node..
> An example is:
> #define vector __attribute__((vector_size(4*sizeof(int) )))
> void g(void)
> {
>  vector int t;
>  t <<= d; /* { dg-error "undeclared" }  */
> }
> --- CUT ---

True, that's a good suggestion.

> How does your patch handle the case where <<= is used and the left
> hand side is an integer type like:
>
> #define vector __attribute__((vector_size(4*sizeof(int) )))
> void g(void)
> {
>  vector int t;
>  int d;
>  d <<= t; /* { dg-error "" }  */
> }
>
> --- CUT ---
> +typedef int v4si __attribute__ ((vector_size (16)));
> I think that would be better if you do this:
> +typedef int v4si __attribute__ ((vector_size (4*sizeof(int) )));
>
> So that it is really v4 and I would name it v4int rather than saying
> the internal GCC mode.
>
> Some more from Joseph S. Myers' review of my patch which is why it was
> not applied yet:
>
> Do I understand correctly that it is also intended that the vectors must
> have the same width and number of elements (but possibly different
> signedness)?

I think we can allow arbitrary integer types as long as the vectors
have the same number of elements.  At least if the Cell or OpenCL
spec do not put other restrictions on the element types.

> If so, the testcases should cover this (errors for different width, errors
> for different number of elements, allowing different signedness, errors if
> either vector is floating point).

That would be indeed nice to have.

> The tests should also verify that signed shift operations where the sign
> bit is involved act as documented in implement-c.texi for individual
> elements, and should test unsigned shift operations where the result
> differs from that of signed shift (right shifting values with the top bit
> set).

It might be the case that if the operation  maps to hardware instructions
the vector hardware behaves differently, so I suppose implement-c.texi
should be adjusted accordingly (or, as those operations are not
standard C, the vector extension documentation should be amended
that the implementation might not follow implement-c.texi?)

>  It would also be good to run tests for different base types and
> vector lengths (after all, the conversion to scalar code could generate
> things such as shifts on types smaller than int that wouldn't arise
> directly in normal C code, so it may not be obvious that it's well-tested
> whether such operations on char or short work correctly at present,
> although certainly valid GIMPLE), and to test shift count vectors that do
> not have all counts equal.

More exhaustive testsuite coverage would be indeed nice.

Richard.

> --- CUT ---
>
> Thanks,
> Andrew Pinski
>
diff mbox

Patch

Index: gcc/doc/extend.texi
===================================================================
--- gcc/doc/extend.texi	(revision 160565)
+++ gcc/doc/extend.texi	(working copy)
@@ -6119,7 +6119,7 @@  produce code that uses 4 @code{SIs}.
 
 The types defined in this manner can be used with a subset of normal C
 operations.  Currently, GCC will allow using the following operators
-on these types: @code{+, -, *, /, unary minus, ^, |, &, ~, %}@.
+on these types: @code{+, -, *, /, unary minus, ^, |, &, ~, %, <<, >>}@.
 
 The operations behave like C++ @code{valarrays}.  Addition is defined as
 the addition of the corresponding elements of the operands.  For
@@ -6135,6 +6135,22 @@  v4si a, b, c;
 c = a + b;
 @end smallexample
 
+Shifting operators @code{<<, >>} support integer vector operands doing
+element-wise shifting.  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 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
+
 Subtraction, multiplication, division, and the logical operations
 operate in a similar manner.  Likewise, the result of using the unary
 minus or complement operators on a vector type is a vector whose
Index: gcc/tree.c
===================================================================
--- gcc/tree.c	(revision 160565)
+++ gcc/tree.c	(working copy)
@@ -1312,6 +1312,23 @@  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 (const tree sc, const tree vectype) 
+{
+  tree t = NULL_TREE;
+  int i, nunits = TYPE_VECTOR_SUBPARTS (vectype);
+
+  gcc_assert (sc == error_mark_node
+              || TREE_TYPE (sc) == TREE_TYPE (vectype));
+
+  for (i = 0; i < nunits; ++i)
+    t = tree_cons (NULL_TREE, sc, t);
+
+  return build_vector (vectype, t);
+}
+
+
 /* Return a new CONSTRUCTOR node whose type is TYPE and whose values
    are in the VEC pointed to by VALS.  */
 tree
Index: gcc/tree.h
===================================================================
--- gcc/tree.h	(revision 160565)
+++ gcc/tree.h	(working copy)
@@ -4020,6 +4020,7 @@  extern tree build_int_cst_type (tree, HO
 extern tree build_int_cst_wide (tree, unsigned HOST_WIDE_INT, HOST_WIDE_INT);
 extern tree build_vector (tree, tree);
 extern tree build_vector_from_ctor (tree, VEC(constructor_elt,gc) *);
+extern tree build_vector_from_val (const tree, const tree);
 extern tree build_constructor (tree, VEC(constructor_elt,gc) *);
 extern tree build_constructor_single (tree, tree, tree);
 extern tree build_constructor_from_list (tree, tree);
Index: gcc/testsuite/gcc.c-torture/execute/vector-shift.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/vector-shift.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/vector-shift.c	(revision 0)
@@ -0,0 +1,47 @@ 
+
+#define vector __attribute__((vector_size(sizeof(int)*4) ))
+
+static vector int allones = {1, 1, 1, 1};
+static vector int allzeros = {0, 0, 0, 0};
+static vector int numbers = {0, 1, 2, 3};
+static vector int numbersleftshiftallones = {0, 2, 4, 6};
+static vector int numbersrightshiftallones = {0, 0, 1, 1};
+
+
+static vector unsigned int uallones = {1, 1, 1, 1};
+static vector unsigned int uallzeros = {0, 0, 0, 0};
+static vector unsigned int unumbers = {0, 1, 2, 3};
+static vector unsigned int unumbersleftshiftallones = {0, 2, 4, 6};
+static vector unsigned int unumbersrightshiftallones = {0, 0, 1, 1};
+
+#define TEST(result, expected) \
+do { \
+  typeof(result) result1 = result; \
+  if(sizeof (result1) != sizeof (expected)) \
+    __builtin_abort (); \
+  if (__builtin_memcmp (&result1, &expected, sizeof(result1)) != 0) \
+    __builtin_abort (); \
+}while (0);
+
+int main(void)
+{
+  vector int result;
+  TEST ((numbers << allzeros), numbers);
+  TEST ((numbers >> allzeros), numbers);
+  TEST((numbers << allones), numbersleftshiftallones);
+  TEST((numbers >> allones), numbersrightshiftallones);
+  /* Test left shift followed by a right shift, numbers should be back as
+     numbers are all small numbers and no lose of precision happens.   */
+  TEST((numbers << allones) >> allones, numbers);
+  
+  
+  
+  TEST ((unumbers << uallzeros), unumbers);
+  TEST ((unumbers >> uallzeros), unumbers);
+  TEST((unumbers << uallones), unumbersleftshiftallones);
+  TEST((unumbers >> uallones), unumbersrightshiftallones);
+  /* Test left shift followed by a right shift, numbers should be back as
+     numbers are all small numbers and no lose of precision happens.   */
+  TEST((unumbers << uallones) >> uallones, unumbers);
+  
+}
Index: gcc/testsuite/gcc.c-torture/execute/vector-shift1.c
===================================================================
--- gcc/testsuite/gcc.c-torture/execute/vector-shift1.c	(revision 0)
+++ gcc/testsuite/gcc.c-torture/execute/vector-shift1.c	(revision 0)
@@ -0,0 +1,16 @@ 
+#define vector __attribute__((vector_size(16) ))
+
+int main (int argc, char *argv[]) {
+  vector short v0 = {argc,2,3,4,5,6,7};
+  vector short v1 = {2,2,2,2,2,2,2};
+  vector short r1,r2,r3,r4;
+  int i = 8;
+
+  r1 = v0 << 1;
+  r2 = 1 << v0;
+
+  r3 = v0 << v1;
+  r4 = v0 >> v1;
+
+  return 0;
+}
Index: gcc/c-typeck.c
===================================================================
--- gcc/c-typeck.c	(revision 160565)
+++ gcc/c-typeck.c	(working copy)
@@ -9373,6 +9373,32 @@  build_binary_op (location_t location, en
 
   objc_ok = objc_compare_types (type0, type1, -3, NULL_TREE);
 
+  /* For most of binary operations in case we have 'vector <op> scalar'
+     or 'scalar <op> vector', we convert a scalar to a vector. For shifts 
+     truncating the shift amount is ok. For constants we just check the 
+     boundaries.  */
+  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 (sc, type1);
+                orig_type0 = type0 = TREE_TYPE (op0);
+                code0 = TREE_CODE (type0);
+              }
+            break;
+
+          default:
+            break;
+        }
+    }
+
   switch (code)
     {
     case PLUS_EXPR:
@@ -9534,7 +9560,21 @@  build_binary_op (location_t location, en
 	 Also set SHORT_SHIFT if shifting rightward.  */
 
     case RSHIFT_EXPR:
-      if ((code0 == INTEGER_TYPE || code0 == FIXED_POINT_TYPE)
+      if (code0 == VECTOR_TYPE && code1 == INTEGER_TYPE
+          && TREE_CODE (TREE_TYPE (type0)) == INTEGER_TYPE)
+        {
+          result_type = type0;
+          converted = 1;
+        }
+      else if (code0 == VECTOR_TYPE && code1 == VECTOR_TYPE
+	  && TREE_CODE (TREE_TYPE (type0)) == INTEGER_TYPE
+          && TREE_CODE (TREE_TYPE (type1)) == INTEGER_TYPE
+          && TYPE_VECTOR_SUBPARTS (type0) == TYPE_VECTOR_SUBPARTS (type1))
+	{
+	  result_type = type0;
+	  converted = 1;
+	}
+      else if ((code0 == INTEGER_TYPE || code0 == FIXED_POINT_TYPE)
 	  && code1 == INTEGER_TYPE)
 	{
 	  if (TREE_CODE (op1) == INTEGER_CST)
@@ -9561,9 +9601,10 @@  build_binary_op (location_t location, en
 
 	  /* Use the type of the value to be shifted.  */
 	  result_type = type0;
-	  /* Convert the shift-count to an integer, regardless of size
-	     of value being shifted.  */
-	  if (TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
+	  /* Convert the non vector shift-count to an integer, regardless
+	     of size of value being shifted.  */
+	  if (TREE_CODE (TREE_TYPE (op1)) != VECTOR_TYPE
+	      && TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
 	    op1 = convert (integer_type_node, op1);
 	  /* Avoid converting op1 to result_type later.  */
 	  converted = 1;
@@ -9571,7 +9612,21 @@  build_binary_op (location_t location, en
       break;
 
     case LSHIFT_EXPR:
-      if ((code0 == INTEGER_TYPE || code0 == FIXED_POINT_TYPE)
+      if (code0 == VECTOR_TYPE && code1 == INTEGER_TYPE
+          && TREE_CODE (TREE_TYPE (type0)) == INTEGER_TYPE)
+        {
+          result_type = type0;
+          converted = 1;
+        }
+      else if (code0 == VECTOR_TYPE && code1 == VECTOR_TYPE
+	  && TREE_CODE (TREE_TYPE (type0)) == INTEGER_TYPE
+          && TREE_CODE (TREE_TYPE (type1)) == INTEGER_TYPE
+          && TYPE_VECTOR_SUBPARTS (type0) == TYPE_VECTOR_SUBPARTS (type1))
+	{
+	  result_type = type0;
+	  converted = 1;
+	}
+      else if ((code0 == INTEGER_TYPE || code0 == FIXED_POINT_TYPE)
 	  && code1 == INTEGER_TYPE)
 	{
 	  if (TREE_CODE (op1) == INTEGER_CST)
@@ -9593,9 +9648,10 @@  build_binary_op (location_t location, en
 
 	  /* Use the type of the value to be shifted.  */
 	  result_type = type0;
-	  /* Convert the shift-count to an integer, regardless of size
-	     of value being shifted.  */
-	  if (TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
+	  /* Convert the non vector shift-count to an integer, regardless
+	     of size of value being shifted.  */
+	  if (TREE_CODE (TREE_TYPE (op1)) != VECTOR_TYPE
+	      && TYPE_MAIN_VARIANT (TREE_TYPE (op1)) != integer_type_node)
 	    op1 = convert (integer_type_node, op1);
 	  /* Avoid converting op1 to result_type later.  */
 	  converted = 1;