Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Andreas Krebbel]

On 09/27/2017 07:30 PM, Sandra Loosemore wrote:
> On 09/27/2017 03:05 AM, Rainer Orth wrote:
>> Hi Andreas,
>>
>>> On 09/27/2017 10:10 AM, Rainer Orth wrote:
 Hi Andreas,

> On 09/26/2017 02:26 PM, Rainer Orth wrote:
>> Hi Andreas,
>>
>>> diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
>>> index 307c726..3acfd85 100644
>>> --- a/gcc/doc/sourcebuild.texi
>>> +++ b/gcc/doc/sourcebuild.texi
>>> @@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
>>>   @item vect_no_align
>>>   Target does not support a vector alignment mechanism.
>>>
>>> +@item vect_no_peel
>>> +Target does not require any loop peeling for alignment purposes.
>>> +
>>>   @item vect_no_int_min_max
>>>   Target does not support a vector min and max instruction on 
>>> @code{int}.
>>
>> please keep the items sorted alphabetically.
>
> The items do not appear to be sorted alphabetically.

 they should be.  Your patch makes the ordering even more random.

 Patch to fix this preapproved ;-)
>>> The items rather appear to be arranged by subject. Does it really make
>>> sense do pull items like this
>>> apart just to have it in alphabetical order?
>>>
>>> @item vect_intfloat_cvt
>>> Target supports conversion from @code{signed int} to @code{float}.
>>>
>>> @item vect_uintfloat_cvt
>>> Target supports conversion from @code{unsigned int} to @code{float}.
>>>
>>> @item vect_floatint_cvt
>>> Target supports conversion from @code{float} to @code{signed int}.
>>>
>>> @item vect_floatuint_cvt
>>> Target supports conversion from @code{float} to @code{unsigned int}.
>>>
>>>
>>> I've added the no_peel item intentionally to the hw_misalign/no_align block.
>>
>> granted, there are some attempts at that, but I find it hard to make my
>> way through that longish list.  The way it is, you have to skip through
>> the whole list beginning to end.  Texinfo seems to have no subsubsection
>> which would allow to make the sub-grouping explicit...
>>
>> Let's hear what Sandra thinks.
> 
> U.  There is no common convention in the GCC documentation and other 
> parts of the manual do deliberately diverge from alphabetization in 
> places.  There's a perpetual tension between putting the most 
> commonly-needed information first vs grouping things by related concepts 
> vs alphabetize vs the tendency of people to insert new items at random 
> places in an existing list regardless of how it's previously been 
> organized.  :-(
> 
> Alphabetical lists are useful when you already know the name of the 
> thing you are searching for, but almost everybody reads the 
> documentation in a web browser or PDF viewer with a search feature 
> nowadays so you can find the term no matter how the list is sorted.  So 
> I'd say we shouldn't alphabetize as a matter of policy if there is some 
> other organization that makes sense.
> 
> In this case, the section is already broken into multiple sublists by 
> topic, most of the sublists are fairly short, and where there's some 
> discernible sort order within the sublists, it seems to be grouping 
> related things together rather than alphabetical.  So I wouldn't insist 
> on alphabetizing this particular sublist either.
> 
> -Sandra

Ok thanks for the clarification. I'll try to fit the documentation updates into 
the existing
structure.  Updated patchset here: 
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01862.html

Bye,

-Andreas-

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Andreas Krebbel]

On 09/26/2017 06:49 PM, Richard Sandiford wrote:
> Andreas Krebbel  writes:
...
> Sorry for the bikeshedding, but how about having a positive test
> like vect_can_peel instead?  ! vect_no... can be hard to read in
> complex conditions.  (There's already that problem with existing
> vect_no...s.)

Done. Updated patch here: 
https://gcc.gnu.org/ml/gcc-patches/2017-09/msg01867.html

Bye,

-Andreas-

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Sandra Loosemore]

On 09/27/2017 03:05 AM, Rainer Orth wrote:

Hi Andreas,


On 09/27/2017 10:10 AM, Rainer Orth wrote:

Hi Andreas,


On 09/26/2017 02:26 PM, Rainer Orth wrote:

Hi Andreas,


diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index 307c726..3acfd85 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
  @item vect_no_align
  Target does not support a vector alignment mechanism.

+@item vect_no_peel
+Target does not require any loop peeling for alignment purposes.
+
  @item vect_no_int_min_max
  Target does not support a vector min and max instruction on @code{int}.


please keep the items sorted alphabetically.


The items do not appear to be sorted alphabetically.


they should be.  Your patch makes the ordering even more random.

Patch to fix this preapproved ;-)

The items rather appear to be arranged by subject. Does it really make
sense do pull items like this
apart just to have it in alphabetical order?

@item vect_intfloat_cvt
Target supports conversion from @code{signed int} to @code{float}.

@item vect_uintfloat_cvt
Target supports conversion from @code{unsigned int} to @code{float}.

@item vect_floatint_cvt
Target supports conversion from @code{float} to @code{signed int}.

@item vect_floatuint_cvt
Target supports conversion from @code{float} to @code{unsigned int}.


I've added the no_peel item intentionally to the hw_misalign/no_align block.


granted, there are some attempts at that, but I find it hard to make my
way through that longish list.  The way it is, you have to skip through
the whole list beginning to end.  Texinfo seems to have no subsubsection
which would allow to make the sub-grouping explicit...

Let's hear what Sandra thinks.


U.  There is no common convention in the GCC documentation and other 
parts of the manual do deliberately diverge from alphabetization in 
places.  There's a perpetual tension between putting the most 
commonly-needed information first vs grouping things by related concepts 
vs alphabetize vs the tendency of people to insert new items at random 
places in an existing list regardless of how it's previously been 
organized.  :-(


Alphabetical lists are useful when you already know the name of the 
thing you are searching for, but almost everybody reads the 
documentation in a web browser or PDF viewer with a search feature 
nowadays so you can find the term no matter how the list is sorted.  So 
I'd say we shouldn't alphabetize as a matter of policy if there is some 
other organization that makes sense.


In this case, the section is already broken into multiple sublists by 
topic, most of the sublists are fairly short, and where there's some 
discernible sort order within the sublists, it seems to be grouping 
related things together rather than alphabetical.  So I wouldn't insist 
on alphabetizing this particular sublist either.


-Sandra

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Rainer Orth]

Hi Andreas,

> On 09/27/2017 10:10 AM, Rainer Orth wrote:
>> Hi Andreas,
>> 
>>> On 09/26/2017 02:26 PM, Rainer Orth wrote:
 Hi Andreas,

> diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
> index 307c726..3acfd85 100644
> --- a/gcc/doc/sourcebuild.texi
> +++ b/gcc/doc/sourcebuild.texi
> @@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
>  @item vect_no_align
>  Target does not support a vector alignment mechanism.
>
> +@item vect_no_peel
> +Target does not require any loop peeling for alignment purposes.
> +
>  @item vect_no_int_min_max
>  Target does not support a vector min and max instruction on @code{int}.

 please keep the items sorted alphabetically.
>>>
>>> The items do not appear to be sorted alphabetically.
>> 
>> they should be.  Your patch makes the ordering even more random.
>> 
>> Patch to fix this preapproved ;-)
> The items rather appear to be arranged by subject. Does it really make
> sense do pull items like this
> apart just to have it in alphabetical order?
>
> @item vect_intfloat_cvt
> Target supports conversion from @code{signed int} to @code{float}.
>
> @item vect_uintfloat_cvt
> Target supports conversion from @code{unsigned int} to @code{float}.
>
> @item vect_floatint_cvt
> Target supports conversion from @code{float} to @code{signed int}.
>
> @item vect_floatuint_cvt
> Target supports conversion from @code{float} to @code{unsigned int}.
>
>
> I've added the no_peel item intentionally to the hw_misalign/no_align block.

granted, there are some attempts at that, but I find it hard to make my
way through that longish list.  The way it is, you have to skip through
the whole list beginning to end.  Texinfo seems to have no subsubsection
which would allow to make the sub-grouping explicit...

Let's hear what Sandra thinks.

Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Andreas Krebbel]

On 09/27/2017 10:10 AM, Rainer Orth wrote:
> Hi Andreas,
> 
>> On 09/26/2017 02:26 PM, Rainer Orth wrote:
>>> Hi Andreas,
>>>
 diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
 index 307c726..3acfd85 100644
 --- a/gcc/doc/sourcebuild.texi
 +++ b/gcc/doc/sourcebuild.texi
 @@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
  @item vect_no_align
  Target does not support a vector alignment mechanism.

 +@item vect_no_peel
 +Target does not require any loop peeling for alignment purposes.
 +
  @item vect_no_int_min_max
  Target does not support a vector min and max instruction on @code{int}.
>>>
>>> please keep the items sorted alphabetically.
>>
>> The items do not appear to be sorted alphabetically.
> 
> they should be.  Your patch makes the ordering even more random.
> 
> Patch to fix this preapproved ;-)
The items rather appear to be arranged by subject. Does it really make sense do 
pull items like this
apart just to have it in alphabetical order?

@item vect_intfloat_cvt
Target supports conversion from @code{signed int} to @code{float}.

@item vect_uintfloat_cvt
Target supports conversion from @code{unsigned int} to @code{float}.

@item vect_floatint_cvt
Target supports conversion from @code{float} to @code{signed int}.

@item vect_floatuint_cvt
Target supports conversion from @code{float} to @code{unsigned int}.


I've added the no_peel item intentionally to the hw_misalign/no_align block.

-Andreas-

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Rainer Orth]

Hi Andreas,

> On 09/26/2017 02:26 PM, Rainer Orth wrote:
>> Hi Andreas,
>> 
>>> diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
>>> index 307c726..3acfd85 100644
>>> --- a/gcc/doc/sourcebuild.texi
>>> +++ b/gcc/doc/sourcebuild.texi
>>> @@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
>>>  @item vect_no_align
>>>  Target does not support a vector alignment mechanism.
>>>
>>> +@item vect_no_peel
>>> +Target does not require any loop peeling for alignment purposes.
>>> +
>>>  @item vect_no_int_min_max
>>>  Target does not support a vector min and max instruction on @code{int}.
>> 
>> please keep the items sorted alphabetically.
>
> The items do not appear to be sorted alphabetically.

they should be.  Your patch makes the ordering even more random.

Patch to fix this preapproved ;-)

Thanks.
Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Andreas Krebbel]

On 09/26/2017 02:26 PM, Rainer Orth wrote:
> Hi Andreas,
> 
>> diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
>> index 307c726..3acfd85 100644
>> --- a/gcc/doc/sourcebuild.texi
>> +++ b/gcc/doc/sourcebuild.texi
>> @@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
>>  @item vect_no_align
>>  Target does not support a vector alignment mechanism.
>>
>> +@item vect_no_peel
>> +Target does not require any loop peeling for alignment purposes.
>> +
>>  @item vect_no_int_min_max
>>  Target does not support a vector min and max instruction on @code{int}.
> 
> please keep the items sorted alphabetically.

The items do not appear to be sorted alphabetically.

-Andreas-

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Richard Sandiford]

Andreas Krebbel  writes:
> - vect_nopeel renamed to vect_no_peel
> - documentation added.
>
> gcc/testsuite/ChangeLog:
>
> 2017-09-26  Andreas Krebbel  
>
>   * doc/sourcebuild.texi: Document vect_no_peel.
>
> gcc/testsuite/ChangeLog:
>
> 2017-09-26  Andreas Krebbel  
>
>   * g++.dg/vect/slp-pr56812.cc: Check vect_nopeel.
>   * lib/target-supports.exp (check_effective_target_vect_nopeel):
>   New proc.

Sorry for the bikeshedding, but how about having a positive test
like vect_can_peel instead?  ! vect_no... can be hard to read in
complex conditions.  (There's already that problem with existing
vect_no...s.)

> -/* { dg-final { scan-tree-dump-times "basic block vectorized" 1 "slp1" } } */
>
> +/* For targets without vector loop peeling the loop becomes cheap
>
> +   enough to be vectorized.  */
>
> +/* { dg-final { scan-tree-dump-times "basic block vectorized" 1 "slp1" { 
> target { ! vect_no_peel }
> } } } */

How about an xfail instead?  Then it'll be noticeable (via an XPASS)
if we fail to vectorise the loop when we should.

Thanks,
Richard

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Rainer Orth]

Hi Andreas,

> diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
> index 307c726..3acfd85 100644
> --- a/gcc/doc/sourcebuild.texi
> +++ b/gcc/doc/sourcebuild.texi
> @@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
>  @item vect_no_align
>  Target does not support a vector alignment mechanism.
>
> +@item vect_no_peel
> +Target does not require any loop peeling for alignment purposes.
> +
>  @item vect_no_int_min_max
>  Target does not support a vector min and max instruction on @code{int}.

please keep the items sorted alphabetically.

Thanks.
Rainer

-- 
-
Rainer Orth, Center for Biotechnology, Bielefeld University

Re: [PATCH 4/5] New target check: vect_nopeel - v2

[Andreas Krebbel]

- vect_nopeel renamed to vect_no_peel
- documentation added.

gcc/testsuite/ChangeLog:

2017-09-26  Andreas Krebbel  

* doc/sourcebuild.texi: Document vect_no_peel.

gcc/testsuite/ChangeLog:

2017-09-26  Andreas Krebbel  

* g++.dg/vect/slp-pr56812.cc: Check vect_nopeel.
* lib/target-supports.exp (check_effective_target_vect_nopeel):
New proc.
---
 gcc/doc/sourcebuild.texi |  3 +++
 gcc/testsuite/g++.dg/vect/slp-pr56812.cc |  4 +++-
 gcc/testsuite/lib/target-supports.exp| 22 ++
 3 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/gcc/doc/sourcebuild.texi b/gcc/doc/sourcebuild.texi
index 307c726..3acfd85 100644
--- a/gcc/doc/sourcebuild.texi
+++ b/gcc/doc/sourcebuild.texi
@@ -1398,6 +1398,9 @@ Target supports a vector misalign access.
 @item vect_no_align
 Target does not support a vector alignment mechanism.

+@item vect_no_peel
+Target does not require any loop peeling for alignment purposes.
+
 @item vect_no_int_min_max
 Target does not support a vector min and max instruction on @code{int}.

diff --git a/gcc/testsuite/g++.dg/vect/slp-pr56812.cc 
b/gcc/testsuite/g++.dg/vect/slp-pr56812.cc
index 80bdcdd..3dbaf76 100644
--- a/gcc/testsuite/g++.dg/vect/slp-pr56812.cc
+++ b/gcc/testsuite/g++.dg/vect/slp-pr56812.cc
@@ -17,4 +17,6 @@ void mydata::Set (float x)
 data[i] = x;

 }



-/* { dg-final { scan-tree-dump-times "basic block vectorized" 1 "slp1" } } */

+/* For targets without vector loop peeling the loop becomes cheap

+   enough to be vectorized.  */

+/* { dg-final { scan-tree-dump-times "basic block vectorized" 1 "slp1" { 
target { ! vect_no_peel }
} } } */

diff --git a/gcc/testsuite/lib/target-supports.exp 
b/gcc/testsuite/lib/target-supports.exp
index 7fdfbbb..31e802d 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -3199,6 +3199,28 @@ proc check_effective_target_vect_floatuint_cvt { } {
 return $et_vect_floatuint_cvt_saved($et_index)
 }

+# Return 1 if peeling for alignment is never profitable on the target
+#
+
+proc check_effective_target_vect_no_peel { } {
+global et_vect_no_peel_saved
+global et_index
+
+if [info exists et_vect_no_peel_saved($et_index)] {
+   verbose "check_effective_target_vect_no_peel: using cached result" 2
+} else {
+   set et_vect_no_peel_saved($et_index) 0
+if { ([istarget s390*-*-*]
+ && [check_effective_target_s390_vx]) } {
+   set et_vect_no_peel_saved($et_index) 1
+}
+}
+
+verbose "check_effective_target_vect_no_peel:\
+returning $et_vect_no_peel_saved($et_index)" 2
+return $et_vect_no_peel_saved($et_index)
+}
+
 # Return 1 if the target supports #pragma omp declare simd, 0 otherwise.
 #
 # This won't change for different subtargets so cache the result.
-- 
2.9.1