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