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.
vector-op-warning-1.diff
Description: Binary data
