Re: New warning for expanded vector operations

[Artem Shinkarov <artyom dot shinkaroff at gmail dot com>]

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.

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