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