On Mon, Oct 10, 2011 at 12:02 PM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov
> <artyom.shinkar...@gmail.com> wrote:
>> On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov
>> <artyom.shinkar...@gmail.com> wrote:
>>> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther
>>> <richard.guent...@gmail.com> wrote:
>>>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov
>>>> <artyom.shinkar...@gmail.com> wrote:
>>>>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther
>>>>> <richard.guent...@gmail.com> wrote:
>>>>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov
>>>>>> <artyom.shinkar...@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?
Thanks,
Artem.
> Thanks,
> Richard.
>
>>
>> Thanks,
>> Artem.
>>
>
vector-op-warning-2.diff
Description: Binary data
