Re: New warning for expanded vector operations

[Artem Shinkarov <artyom dot shinkaroff at gmail dot com>]

On Tue, Oct 11, 2011 at 11:52 AM, Richard Guenther
<richard.guenther@gmail.com> wrote:
> On Mon, Oct 10, 2011 at 3:21 PM, Artem Shinkarov
> <artyom.shinkaroff@gmail.com> wrote:
>> On Mon, Oct 10, 2011 at 12:02 PM, Richard Guenther
>> <richard.guenther@gmail.com> wrote:
>>> 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).
>>
>> Ok, Fixed.
>>>
>>> @@ -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?
>>
>> Most likely it means sower. ?Eventually it is a different kind of the
>> expansion. ?This situation could happen when you work with MMX
>> vectors, and by some reason instead of a single MMX operation, you
>> will have register operation + masking.
>>>
>>> @@ -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.
>>
>> Ok, moved to c-typeck.c.
>>
>>
>> The new version is in the attachment. ?Boostrapped on x86_64-apple-darwin10.8.0.
>> Ok?
>
> Ok with
>
> @@ -2934,7 +2934,8 @@ c_build_vec_perm_expr (location_t loc, t
>
> ? if (!wrap)
> ? ? ret = c_wrap_maybe_const (ret, true);
> -
> +
> + ?SET_EXPR_LOCATION (ret, loc);
> ? return ret;
>
> instead of this use build3_loc (loc, ...) when building the VEC_PERM_EXPR
> in the line before this hunk.
>
> Thanks,
> Richard.
>
>>
>> Thanks,
>> Artem.
>>
>>
>>> Thanks,
>>> Richard.
>>>
>>>>
>>>> Thanks,
>>>> Artem.
>>>>
>>>
>>
>

Committed with the revision 179807.


Artem.