Updated patch with spaces, etc according to check_GNU_style.sh Put guard as per Tobias' request.
Is it Ok?
On Thu, Nov 21, 2013 at 6:18 PM, Sergey Ostanevich <sergos....@gmail.com> wrote:
> 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.
patch6
Description: Binary data
