Re: New warning for expanded vector operations

[Artem Shinkarov <artyom.shinkaroff@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6389 bytes --]

On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov
>>> <artyom.shinkaroff@gmail.com> wrote:
>>>> Hi
>>>>
>>>> Here is a patch to inform a programmer about the expanded vector operation.
>>>> Bootstrapped on x86-unknown-linux-gnu.
>>>>
>>>> ChangeLog:
>>>>
>>>>        * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to
>>>>          produce the warning.
>>>>          (expand_vector_parallel): Adjust to produce the warning.
>>>
>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file.
>>
>> Sure, sorry.
>>
>>>>          (lower_vec_shuffle): Adjust to produce the warning.
>>>>        * gcc/common.opt: New warning Wvector-operation-expanded.
>>>>        * gcc/doc/invoke.texi: Document the wawning.
>>>>
>>>>
>>>> Ok?
>>>
>>> I don't like the name -Wvector-operation-expanded.  We emit a
>>> similar warning for missed inline expansions with -Winline, so
>>> maybe -Wvector-extensions (that's the name that appears
>>> in the C extension documentation).
>>
>> Hm, I don't care much about the name, unless it gets clear what the
>> warning is used for.  I am not really sure that Wvector-extensions
>> makes it clear.  Also, I don't see anything bad if the warning will
>> pop up during the vectorisation. Any vector operation performed
>> outside the SIMD accelerator looks suspicious, because it actually
>> doesn't improve performance.  Such a warning during the vectorisation
>> could mean that a programmer forgot some flag, or the constant
>> propagation failed to deliver a constant, or something else.
>>
>> Conceptually the text I am producing is not really a warning, it is
>> more like an information, but I am not aware of the mechanisms that
>> would allow me to introduce a flag triggering inform () or something
>> similar.
>>
>> What I think we really need to avoid is including this warning in the
>> standard Ox.
>>
>>> +  location_t loc = gimple_location (gsi_stmt (*gsi));
>>> +
>>> +  warning_at (loc, OPT_Wvector_operation_expanded,
>>> +             "vector operation will be expanded piecewise");
>>>
>>>   v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta);
>>>   for (i = 0; i < nunits;
>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter
>>>   tree result, compute_type;
>>>   enum machine_mode mode;
>>>   int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD;
>>> +  location_t loc = gimple_location (gsi_stmt (*gsi));
>>> +
>>> +  warning_at (loc, OPT_Wvector_operation_expanded,
>>> +             "vector operation will be expanded in parallel");
>>>
>>> what's the difference between 'piecewise' and 'in parallel'?
>>
>> Parallel is a little bit better for performance than piecewise.
>
> I see.  That difference should probably be documented, maybe with
> an example.
>
> Richard.
>
>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter
>>>  {
>>>   int parts_per_word = UNITS_PER_WORD
>>>                       / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1);
>>> +  location_t loc = gimple_location (gsi_stmt (*gsi));
>>>
>>>   if (INTEGRAL_TYPE_P (TREE_TYPE (type))
>>>       && parts_per_word >= 4
>>>       && TYPE_VECTOR_SUBPARTS (type) >= 4)
>>> -    return expand_vector_parallel (gsi, f_parallel,
>>> -                                  type, a, b, code);
>>> +    return expand_vector_parallel (gsi, f_parallel, type, a, b, code);
>>>   else
>>> -    return expand_vector_piecewise (gsi, f,
>>> -                                   type, TREE_TYPE (type),
>>> -                                   a, b, code);
>>> +    return expand_vector_piecewise (gsi, f, type,
>>> +                                   TREE_TYPE (type), a, b, code);
>>>  }
>>>
>>>  /* Check if vector VEC consists of all the equal elements and
>>>
>>> unless i miss something loc is unused here.  Please avoid random
>>> whitespace changes (just review your patch yourself before posting
>>> and revert pieces that do nothing).
>>
>> Yes you are right, sorry.
>>
>>> +@item -Wvector-operation-expanded
>>> +@opindex Wvector-operation-expanded
>>> +@opindex Wno-vector-operation-expanded
>>> +Warn if vector operation is not implemented via SIMD capabilities of the
>>> +architecture. Mainly useful for the performance tuning.
>>>
>>> I'd mention that this is for vector operations as of the C extension
>>> documented in "Vector Extensions".
>>>
>>> The vectorizer can produce some operations that will need further
>>> lowering - we probably should make sure to _not_ warn about those.
>>> Try running the vect.exp testsuite with the new warning turned on
>>> (eventually disabling SSE), like with
>>>
>>> obj/gcc> make check-gcc
>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse
>>> vect.exp"
>>
>> Again, see the comment above. I think, if the warning can be triggered
>> only manually, then we are fine. But I'll check anyway how many
>> warnings I'll get from vect.exp.
>>
>>>> P.S. It is hard to write a reasonable testcase for the patch, because
>>>> one needs to guess which architecture would expand a given vector
>>>> operation. But the patch is trivial.
>>>
>>> You can create an aritificial large vector type for example, or put a
>>> testcase under gcc.target/i386 and disable SSE.  We should have
>>> a testcase for this.
>>
>> Yeah, disabling SSE should help.
>>
>>
>> Thanks,
>> Artem.
>>> Thanks,
>>> Richard.
>>>
>>
>

New version of the patch in the attachment with the test-cases.
Bootstrapped on  x86_64-apple-darwin10.8.0.
Currently is being tested.


Richard, I've checked the vect.exp case, as you suggested.  It caused
a lot of failures, but not because of the new warning.  The main
reason is -mno-sse.  The target is capable to vectorize, so the dg
option expects tests to pass, but the artificial option makes them
fail.  Checking the new warning on vect.exp without -mno-sse, it
didn't cause any new failures.  Anyway, we should be pretty much safe,
cause the warning is not a part of -O3.

Thanks,
Artem.

[-- Attachment #2: vector-op-warning-1.diff --]
[-- Type: application/octet-stream, Size: 9719 bytes --]

Index: gcc/doc/invoke.texi
===================================================================
--- gcc/doc/invoke.texi	(revision 179636)
+++ gcc/doc/invoke.texi	(working copy)
@@ -271,7 +271,8 @@ Objective-C and Objective-C++ Dialects}.
 -Wunused-label  -Wunused-local-typedefs -Wunused-parameter @gol
 -Wno-unused-result -Wunused-value @gol -Wunused-variable @gol
 -Wunused-but-set-parameter -Wunused-but-set-variable @gol
--Wvariadic-macros -Wvla -Wvolatile-register-var  -Wwrite-strings}
+-Wvariadic-macros -Wvector-operation-performance -Wvla 
+-Wvolatile-register-var  -Wwrite-strings}
 
 @item C and Objective-C-only Warning Options
 @gccoptlist{-Wbad-function-cast  -Wmissing-declarations @gol
@@ -4535,6 +4536,18 @@ Warn if variadic macros are used in peda
 alternate syntax when in pedantic ISO C99 mode.  This is default.
 To inhibit the warning messages, use @option{-Wno-variadic-macros}.
 
+@item -Wvector-operation-performance
+@opindex Wvector-operation-performance
+@opindex Wno-vector-operation-performance
+Warn if vector operation is not implemented via SIMD capabilities of the
+architecture.  Mainly useful for the performance tuning.
+Vector operation can be implemented @code{piecewise} which means that the
+scalar operation is performed on every vector element; 
+@code{in parallel} which means that the vector operation is implemented
+using scalars of wider type, which normally is more performance efficient;
+and @code{as a single scalar} which means that vector fits into a
+scalar type.
+
 @item -Wvla
 @opindex Wvla
 @opindex Wno-vla
Index: gcc/testsuite/gcc.target/i386/warn-vect-op-3.c
===================================================================
--- gcc/testsuite/gcc.target/i386/warn-vect-op-3.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/warn-vect-op-3.c	(revision 0)
@@ -0,0 +1,21 @@
+/* { dg-do compile }  */
+/* { dg-options "-mno-sse -Wvector-operation-performance" }  */
+#define vector(elcount, type)  \
+__attribute__((vector_size((elcount)*sizeof(type)))) type
+
+int main (int argc, char *argv[])
+{
+  vector (8, short) v0 = {argc, 1, 15, 38, 12, -1, argc, 2};
+  vector (8, short) v1 = {-4, argc, 2, 11, 1, 17, -8, argc};
+  vector (8, short) res[] = 
+  {
+    v0 + v1,	      /* { dg-warning "expanded in parallel" }  */
+    v0 - v1,          /* { dg-warning "expanded in parallel" }  */
+    v0 > v1,          /* { dg-warning "expanded piecewise" }  */
+    v0 & v1,          /* { dg-warning "expanded in parallel" }  */
+    __builtin_shuffle (v0, v1),	      /* { dg-warning "expanded piecewise" }  */
+    __builtin_shuffle (v0, v1, v1)    /* { dg-warning "expanded piecewise" }  */
+  };
+  
+  return res[argc][argc];
+}
Index: gcc/testsuite/gcc.target/i386/warn-vect-op-1.c
===================================================================
--- gcc/testsuite/gcc.target/i386/warn-vect-op-1.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/warn-vect-op-1.c	(revision 0)
@@ -0,0 +1,21 @@
+/* { dg-do compile }  */
+/* { dg-options "-mno-sse -Wvector-operation-performance" }  */
+#define vector(elcount, type)  \
+__attribute__((vector_size((elcount)*sizeof(type)))) type
+
+int main (int argc, char *argv[])
+{
+  vector (4, int) v0 = {argc, 1, 15, 38};
+  vector (4, int) v1 = {-4, argc, 2, 11};
+  vector (4, int) res[] = 
+  {
+    v0 + v1,	  /* { dg-warning "expanded piecewise" }  */
+    v0 - v1,	  /* { dg-warning "expanded piecewise" }  */
+    v0 > v1,	  /* { dg-warning "expanded piecewise" }  */
+    v0 & v1,	  /* { dg-warning "expanded in parallel" }  */
+    __builtin_shuffle (v0, v1),	    /* { dg-warning "expanded piecewise" }  */
+    __builtin_shuffle (v0, v1, v1)  /* { dg-warning "expanded piecewise" }  */  
+  };
+
+  return res[argc][argc];
+}
Index: gcc/testsuite/gcc.target/i386/warn-vect-op-2.c
===================================================================
--- gcc/testsuite/gcc.target/i386/warn-vect-op-2.c	(revision 0)
+++ gcc/testsuite/gcc.target/i386/warn-vect-op-2.c	(revision 0)
@@ -0,0 +1,23 @@
+/* { dg-do compile }  */
+/* { dg-options "-mno-sse -Wvector-operation-performance" }  */
+#define vector(elcount, type)  \
+__attribute__((vector_size((elcount)*sizeof(type)))) type
+
+int main (int argc, char *argv[])
+{
+  vector (16, signed char) v0 = {argc, 1, 15, 38, 12, -1, argc, 2, 
+				 argc, 1, 15, 38, 12, -1, argc, 2};
+  vector (16, signed char) v1 = {-4, argc, 2, 11, 1, 17, -8, argc,
+				 argc, 1, 15, 38, 12, -1, argc, 2};
+  vector (16, signed char) res[] = 
+  {
+    v0 + v1,		  /* { dg-warning "expanded in parallel" }  */
+    v0 - v1,              /* { dg-warning "expanded in parallel" }  */
+    v0 > v1,              /* { dg-warning "expanded piecewise" }  */
+    v0 & v1,              /* { dg-warning "expanded in parallel" }  */
+    __builtin_shuffle (v0, v1),        /* { dg-warning "expanded piecewise" }  */
+    __builtin_shuffle (v0, v1, v1)     /* { dg-warning "expanded piecewise" }  */
+  };
+ 
+  return res[argc][argc];
+}
Index: gcc/common.opt
===================================================================
--- gcc/common.opt	(revision 179636)
+++ gcc/common.opt	(working copy)
@@ -694,6 +694,10 @@ Wcoverage-mismatch
 Common Var(warn_coverage_mismatch) Init(1) Warning
 Warn in case profiles in -fprofile-use do not match
 
+Wvector-operation-performance
+Common Var(warn_vector_operation_performance) Warning
+Warn when a vector operation is compiled outside the SIMD
+
 Xassembler
 Driver Separate
 
Index: gcc/tree-vect-generic.c
===================================================================
--- gcc/tree-vect-generic.c	(revision 179636)
+++ gcc/tree-vect-generic.c	(working copy)
@@ -235,6 +235,14 @@ expand_vector_piecewise (gimple_stmt_ite
   int delta = tree_low_cst (part_width, 1)
 	      / tree_low_cst (TYPE_SIZE (TREE_TYPE (type)), 1);
   int i;
+  location_t loc = gimple_location (gsi_stmt (*gsi));
+
+  if (gimple_expr_type (gsi_stmt (*gsi)) == type)
+    warning_at (loc, OPT_Wvector_operation_performance,
+		"vector operation will be expanded piecewise");
+  else
+    warning_at (loc, OPT_Wvector_operation_performance,
+		"vector operation will be expanded in parallel");
 
   v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta);
   for (i = 0; i < nunits;
@@ -260,6 +268,7 @@ expand_vector_parallel (gimple_stmt_iter
   tree result, compute_type;
   enum machine_mode mode;
   int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD;
+  location_t loc = gimple_location (gsi_stmt (*gsi));
 
   /* We have three strategies.  If the type is already correct, just do
      the operation an element at a time.  Else, if the vector is wider than
@@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter
       mode = mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT, 0);
       compute_type = lang_hooks.types.type_for_mode (mode, 1);
       result = f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE, code);
+      warning_at (loc, OPT_Wvector_operation_performance,
+	          "vector operation will be expanded with a "
+		  "single scalar operation");
     }
 
   return result;
@@ -308,7 +320,7 @@ expand_vector_addition (gimple_stmt_iter
     return expand_vector_parallel (gsi, f_parallel,
 				   type, a, b, code);
   else
-    return expand_vector_piecewise (gsi, f,
+    return expand_vector_piecewise (gsi, f, 
 				    type, TREE_TYPE (type),
 				    a, b, code);
 }
@@ -400,8 +412,8 @@ expand_vector_operation (gimple_stmt_ite
       case PLUS_EXPR:
       case MINUS_EXPR:
         if (!TYPE_OVERFLOW_TRAPS (type))
-          return expand_vector_addition (gsi, do_binop, do_plus_minus, type,
-		      		         gimple_assign_rhs1 (assign),
+	  return expand_vector_addition (gsi, do_binop, do_plus_minus, type,
+					 gimple_assign_rhs1 (assign),
 					 gimple_assign_rhs2 (assign), code);
 	break;
 
@@ -626,10 +638,14 @@ lower_vec_shuffle (gimple_stmt_iterator 
   tree constr, t, si, i_val;
   tree vec0tmp = NULL_TREE, vec1tmp = NULL_TREE, masktmp = NULL_TREE;
   bool two_operand_p = !operand_equal_p (vec0, vec1, 0);
+  location_t loc = gimple_location (gsi_stmt (*gsi));
   unsigned i;
 
   if (expand_vec_shuffle_expr_p (TYPE_MODE (vect_type), vec0, vec1, mask))
     return;
+  
+  warning_at (loc, OPT_Wvector_operation_performance,
+              "vector shuffling operation will be expanded piecewise");
 
   v = VEC_alloc (constructor_elt, gc, elements);
   for (i = 0; i < elements; i++)
Index: gcc/c-parser.c
===================================================================
--- gcc/c-parser.c	(revision 179636)
+++ gcc/c-parser.c	(working copy)
@@ -6533,17 +6533,22 @@ c_parser_postfix_expression (c_parser *p
 	      }
 
 	    if (VEC_length (c_expr_t, cexpr_list) == 2)
-	      expr.value =
-		c_build_vec_shuffle_expr
-		  (loc, VEC_index (c_expr_t, cexpr_list, 0)->value,
-		   NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value);
-
+	      {
+		expr.value =
+		  c_build_vec_shuffle_expr
+		    (loc, VEC_index (c_expr_t, cexpr_list, 0)->value,
+		     NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value);
+		SET_EXPR_LOCATION (expr.value, loc);
+	      }
 	    else if (VEC_length (c_expr_t, cexpr_list) == 3)
-	      expr.value =
-		c_build_vec_shuffle_expr
-		  (loc, VEC_index (c_expr_t, cexpr_list, 0)->value,
-		   VEC_index (c_expr_t, cexpr_list, 1)->value,
-		   VEC_index (c_expr_t, cexpr_list, 2)->value);
+	      {
+		expr.value =
+		  c_build_vec_shuffle_expr
+		    (loc, VEC_index (c_expr_t, cexpr_list, 0)->value,
+		     VEC_index (c_expr_t, cexpr_list, 1)->value,
+		     VEC_index (c_expr_t, cexpr_list, 2)->value);
+		SET_EXPR_LOCATION (expr.value, loc);
+	      }
 	    else
 	      {
 		error_at (loc, "wrong number of arguments to "