This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [gomp4 simd, RFC] Simple fix to override vectorization cost estimation.


Tobias,


> When I understand the patch correctly, the warning is shown in two cases:
> a) When the loop could be vectorized but the cost model prevented it
> b) When the loop couldn't be vectorized because of other reasons (e.g. not
> vectorizable because of conditional loop exits, incomplete vectorization
> support by the compiler etc.)
>
> Do I correctly understand the warning? I am asking because the *opt and
> *texi wording suggests that only (a) is the case. - I cannot test as the
> patch cannot be applied with heavy editing (removal of additional line
> breaks, taking care of tabs converted into spaces).

I believe it's only for a) case, since warning stays along with the cost
model report that says only about relative scalar and vector costs of
iteration. The case of exits and vectorization capabilities is handled earlier,
since we have some vector code here.

Will try to attach the patch instead of copy-paste here.

>
> Regarding the warning, I think it sounds a bit colloquial and as if the
> location information is not available. What do you think of "loop with simd
> directive not vectorized" or concise not fully correct: "simd loop not
> vectorized"?

took one of yours.

>
> Additionally, shouldn't that be guarded by "if (warn_openmp_simd &&"?
> Otherwise the flag status isn't used at all in the whole patch.

This is strange to me, since it worked as I pass the OPT_Wopenmp_simd
to the warning_at (). It does:
   show warinig with -Wopenmp-simd
   doesn't show warning with -Wall -Wno-openmp-simd

>
>> +Wopenmp-simd
>> +C C++ Var(warn_openmp_simd) Warning EnabledBy(Wall)
>> +Warn about simd directive is overridden by vectorizer cost model
>
>
> Wording wise, I'd prefer something like:
> "Warn if an simd directive is overridden by the vectorizer cost model"
>
> (Or is it "a simd"? Where are the native speakers when one needs them?)

damn, right! I believe 'a' since simd starts with consonant.

>
> However, in light of my question above, shouldn't it be "Warn if a loop with
> simd directive is not vectorized"?
>
>
>
>> +fsimd-cost-model=
>> +Common Joined RejectNegative Enum(vect_cost_model)
>> Var(flag_simd_cost_model) Init(VECT_COST_MODEL_UNLIMITED)
>> +Specifies the vectorization cost model for code marked with simd
>> directive
>
>
> I think an article is lacking before "simd".

done.

>
>
>> +@item -Wopenmp-simd
>> +@opindex Wopenm-simd
>> +Warn if vectorizer cost model overrides simd directive from user.
>
>
> I think that can be expanded a bit. One could also mention OpenMP/Cilk Plus
> explicitly. Maybe like:  "Warn if the vectorizer cost model overrides the
> OpenMP and Cilk Plus simd directives of the user."
>

done.

> Or if my reading above is correct, how about something like: "Warn if a loop
> with OpenMP or Cilk Plus simd directive is not vectorized. If only the cost
> model prevented the vectorization, the @option{-fsimd-cost-model} option can
> be used to force the vectorization."
>
> Which brings me to my next point: -fvect-cost-model= is not documented. I
> think some words would be helpful, especially about the valid arguments, the
> default and how it interacts with -fvect-cost-model=.

done.

>
>
>> --- a/gcc/fortran/lang.opt
>> +++ b/gcc/fortran/lang.opt
>>
>> +Wopenmp-simd
>> +Fortran Warning
>> +; Documented in C
>
> ("Warning" is also not needed as it is taken from c-family/*opt, but it
> shouldn't harm either.)

done.

Sergos

        * common.opt: Added new option -fsimd-cost-model.
        * tree-vectorizer.h (unlimited_cost_model): Interface update
        to rely on particular loop info.
        * tree-vect-data-refs.c (vect_peeling_hash_insert): Update to
        unlimited_cost_model call according to new interface.
        (vect_peeling_hash_choose_best_peeling): Ditto.
        (vect_enhance_data_refs_alignment): Ditto.
        * tree-vect-slp.c: Ditto.
        * tree-vect-loop.c (vect_estimate_min_profitable_iters): Ditto,
        plus issue a warning in case cost model overrides users' directive.
        * c.opt: add openmp-simd warning.
        * lang.opt: Ditto.
        * doc/invoke.texi: Added new openmp-simd warning.

Attachment: patch3
Description: Binary data


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]