This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: New warning for expanded vector operations


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]