New warning for expanded vector operations

[Artem Shinkarov]

On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov
<artyom.shinkaroff@gmail.com> wrote:
> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther
> <richard.guenther@gmail.com> wrote:
>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov
>> <artyom.shinkaroff@gmail.com> wrote:
>>> 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.
>>
>> 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?


Thanks,
Artem.