Re: New warning for expanded vector operations

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov
<artyom.shinkaroff@gmail.com> wrote:
> On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> 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.
>>
>
> Successfully regression-tested on x86_64-apple-darwin10.8.0.
>
> ChangeLog:
> ? ? ? ?gcc/
> ? ? ? ?* doc/invoke.texi: Document new warning.
> ? ? ? ?* common.opt (Wvector-operation-performance): Define new warning.
> ? ? ? ?* tree-vect-generic.c (expand_vector_piecewise): Warn about expanded
> ? ? ? ?vector operation.
> ? ? ? ?(exapnd_vector_parallel): Warn about expanded vector operation.
> ? ? ? ?(lower_vec_shuffle): Warn about expanded vector operation.
> ? ? ? ?* c-parser.c (c_parser_postfix_expression): Assign correct location
> ? ? ? ?when creating VEC_SHUFFLE_EXPR.
>
> ? ? ? ?gcc/testsuite/
> ? ? ? ?* gcc.target/i386/warn-vect-op-3.c: New test.
> ? ? ? ?* gcc.target/i386/warn-vect-op-1.c: New test.
> ? ? ? ?* gcc.target/i386/warn-vect-op-2.c: New test.
>
> Ok for trunk?

+  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");

we should not check for exact type equivalence here.  Please
use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type)
instead.  We could also consider to pass down the kind of lowering
from the caller (or warn in the callers).

@@ -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");

That means it will be fast, no?  Why warn for it at all?

@@ -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);
 }

You add trailing space here ... (please review your patches yourself
for this kind of errors)

+             {
+               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);

That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_expr.
If then that routine needs fixing.

Thanks,
Richard.

>
> Thanks,
> Artem.
>