On Tue, Oct 11, 2011 at 11:52 AM, Richard Guenther
<richard.guent...@gmail.com> wrote:
> On Mon, Oct 10, 2011 at 3:21 PM, Artem Shinkarov
> <artyom.shinkar...@gmail.com> wrote:
>> 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?
>
> 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.
