PowerPC: Implement TARGET_ATOMIC_ASSIGN_EXPAND_FENV
diff mbox

Message ID alpine.DEB.1.10.1409031436470.27075@tp.orcam.me.uk
State Accepted
Headers show

Commit Message

Maciej W. Rozycki Sept. 3, 2014, 2:01 p.m. UTC
On Tue, 2 Sep 2014, Adhemerval Zanella wrote:

> Ping.
> 
> On 19-08-2014 13:54, Adhemerval Zanella wrote:
> > Ping.
> >
> > On 06-08-2014 17:21, Adhemerval Zanella wrote:
> >> On 01-08-2014 12:31, Joseph S. Myers wrote:
> >>> On Thu, 31 Jul 2014, David Edelsohn wrote:
> >>>
> >>>> Thanks for implementing the FENV support.  The patch generally looks 
> >>>> good to me.
> >>>>
> >>>> My one concern is a detail in the implementation of "update". I do not
> >>>> have enough experience with GENERIC to verify the details and it seems
> >>>> like it is missing building an outer COMPOUND_EXPR containing
> >>>> update_mffs and the CALL_EXPR for update mtfsf.
> >>> I suppose what's actually odd there is that you have
> >>>
> >>> +  tree update_mffs = build2 (MODIFY_EXPR, void_type_node, old_fenv, call_mffs);
> >>> +
> >>> +  tree old_llu = build1 (VIEW_CONVERT_EXPR, uint64_type_node, update_mffs);
> >>>
> >>> so you build a MODIFY_EXPR in void_type_node but then convert it with a 
> >>> VIEW_CONVERT_EXPR.  If you'd built the MODIFY_EXPR in double_type_node 
> >>> then the VIEW_CONVERT_EXPR would be meaningful (the value of an assignment 
> >>> a = b being the new value of a), but reinterpreting a void value doesn't 
> >>> make sense.  Or you could probably just use call_mffs directly in the 
> >>> VIEW_CONVERT_EXPR without explicitly creating the old_fenv variable.
> >>>
> >> Thanks for the review Josephm.  I have changed to avoid the void reinterpretation
> >> and use call_mffs directly.  I have also removed the the mask generation in 'clear'
> >> from your previous message, it is now reusing the mas used in feholdexcept.  The 
> >> testcase patch is the same as before.
> >>
> >> Checked on both linux-powerpc64/powerpc64le and no regressions found.
> >>
> >> --
> >>
> >> 2014-08-06  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
> >>
> >> gcc:
> >> 	* config/rs6000/rs6000.c (rs6000_atomic_assign_expand_fenv): New
> >> 	function.
> >>
> >> gcc/testsuite:
> >> 	* gcc.dg/atomic/c11-atomic-exec-5.c
> >> 	(test_main_long_double_add_overflow): Define and run only for
> >> 	LDBL_MANT_DIG != 106.
> >> 	(test_main_complex_long_double_add_overflow): Likewise.
> >> 	(test_main_long_double_sub_overflow): Likewise.
> >> 	(test_main_complex_long_double_sub_overflow): Likewise.

 FWIW I pushed it through regression testing across my usual set of 
powerpc-linux-gnu multilibs with the results (for c11-atomic-exec-5.c) as 
follows:

-mcpu=603e						PASS
-mcpu=603e -msoft-float					UNSUPPORTED
-mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe	UNSUPPORTED
-mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe	UNSUPPORTED
-mcpu=7400 -maltivec -mabi=altivec			PASS
-mcpu=e6500 -maltivec -mabi=altivec			PASS
-mcpu=e5500 -m64					PASS
-mcpu=e6500 -m64 -maltivec -mabi=altivec		PASS

(floating-point environment is of course unsupported for soft-float 
targets and for the SPE FPU another change is required to implement 
floating-point environment handling to complement one proposed here).  
No regressions otherwise.

 While at it, may I propose another change on top of this?

 I've noticed the test case is rather slow, it certainly takes much more 
time than the average one, I've seen elapsed times of well over a minute 
on reasonably fast hardware and occasionally a timeout midway through even 
though the test case was otherwise progressing just fine.  I think lock 
contention or unrelated system activity such as hardware interrupts (think 
a busy network!) may contribute to it for systems using LL/SC loops for 
atomicity.

 So I think the default timeout that's used for really quick tests should 
be extended a bit.  I propose a factor of 2, just not to make it too 
excessive, at least for the beginning (maybe it'll have to be higher 
eventually).

 OK?

2014-09-03  Maciej W. Rozycki  <macro@codesourcery.com>

	gcc/testsuite/
	* gcc.dg/atomic/c11-atomic-exec-5.c (dg-timeout-factor): New 
	setting.

  Maciej

gcc-test-c11-atomic-exec-5-timeout-factor.diff

Comments

Joseph Myers Sept. 3, 2014, 3:48 p.m. UTC | #1
On Wed, 3 Sep 2014, Maciej W. Rozycki wrote:

> (floating-point environment is of course unsupported for soft-float 
> targets and for the SPE FPU another change is required to implement 
> floating-point environment handling to complement one proposed here).  

Support for SPE will depend on the C library just as soft-float support 
will, because of the need to have trapping on exceptions other than 
"inexact" enabled in the processor at all times with the kernel then using 
the prctl settings to determine whether that trap is for emulation or to 
produce SIGFPE.

(The relevant support is in glibc 2.19 for soft-float and e500, in the 
form of __atomic_feholdexcept, __atomic_feclearexcept and 
__atomic_feupdateenv functions.  I intend to implement the GCC side - 
conditional on being configured for glibc 2.19 or later on the target, as 
specified with --with-glibc-version or detected by configure's examination 
of target headers - once the hard-float support is in GCC.  I believe the 
support in question will be identical for soft-float and e500, since it 
will be calling libc functions instead of manipulating processor state.)
Adhemerval Zanella Sept. 4, 2014, 6:40 p.m. UTC | #2
On 03-09-2014 11:01, Maciej W. Rozycki wrote:
> On Tue, 2 Sep 2014, Adhemerval Zanella wrote:
>
>> Ping.
>>
>> On 19-08-2014 13:54, Adhemerval Zanella wrote:
>>> Ping.
>>>
>>> On 06-08-2014 17:21, Adhemerval Zanella wrote:
>>>> On 01-08-2014 12:31, Joseph S. Myers wrote:
>>>>> On Thu, 31 Jul 2014, David Edelsohn wrote:
>>>>>
>>>>>> Thanks for implementing the FENV support.  The patch generally looks 
>>>>>> good to me.
>>>>>>
>>>>>> My one concern is a detail in the implementation of "update". I do not
>>>>>> have enough experience with GENERIC to verify the details and it seems
>>>>>> like it is missing building an outer COMPOUND_EXPR containing
>>>>>> update_mffs and the CALL_EXPR for update mtfsf.
>>>>> I suppose what's actually odd there is that you have
>>>>>
>>>>> +  tree update_mffs = build2 (MODIFY_EXPR, void_type_node, old_fenv, call_mffs);
>>>>> +
>>>>> +  tree old_llu = build1 (VIEW_CONVERT_EXPR, uint64_type_node, update_mffs);
>>>>>
>>>>> so you build a MODIFY_EXPR in void_type_node but then convert it with a 
>>>>> VIEW_CONVERT_EXPR.  If you'd built the MODIFY_EXPR in double_type_node 
>>>>> then the VIEW_CONVERT_EXPR would be meaningful (the value of an assignment 
>>>>> a = b being the new value of a), but reinterpreting a void value doesn't 
>>>>> make sense.  Or you could probably just use call_mffs directly in the 
>>>>> VIEW_CONVERT_EXPR without explicitly creating the old_fenv variable.
>>>>>
>>>> Thanks for the review Josephm.  I have changed to avoid the void reinterpretation
>>>> and use call_mffs directly.  I have also removed the the mask generation in 'clear'
>>>> from your previous message, it is now reusing the mas used in feholdexcept.  The 
>>>> testcase patch is the same as before.
>>>>
>>>> Checked on both linux-powerpc64/powerpc64le and no regressions found.
>>>>
>>>> --
>>>>
>>>> 2014-08-06  Adhemerval Zanella  <azanella@linux.vnet.ibm.com>
>>>>
>>>> gcc:
>>>> 	* config/rs6000/rs6000.c (rs6000_atomic_assign_expand_fenv): New
>>>> 	function.
>>>>
>>>> gcc/testsuite:
>>>> 	* gcc.dg/atomic/c11-atomic-exec-5.c
>>>> 	(test_main_long_double_add_overflow): Define and run only for
>>>> 	LDBL_MANT_DIG != 106.
>>>> 	(test_main_complex_long_double_add_overflow): Likewise.
>>>> 	(test_main_long_double_sub_overflow): Likewise.
>>>> 	(test_main_complex_long_double_sub_overflow): Likewise.
>  FWIW I pushed it through regression testing across my usual set of 
> powerpc-linux-gnu multilibs with the results (for c11-atomic-exec-5.c) as 
> follows:
>
> -mcpu=603e						PASS
> -mcpu=603e -msoft-float					UNSUPPORTED
> -mcpu=8540 -mfloat-gprs=single -mspe=yes -mabi=spe	UNSUPPORTED
> -mcpu=8548 -mfloat-gprs=double -mspe=yes -mabi=spe	UNSUPPORTED
> -mcpu=7400 -maltivec -mabi=altivec			PASS
> -mcpu=e6500 -maltivec -mabi=altivec			PASS
> -mcpu=e5500 -m64					PASS
> -mcpu=e6500 -m64 -maltivec -mabi=altivec		PASS

Thanks for testing it, I'll to add these permutations on my own testbench.

>
> (floating-point environment is of course unsupported for soft-float 
> targets and for the SPE FPU another change is required to implement 
> floating-point environment handling to complement one proposed here).  
> No regressions otherwise.
>
>  While at it, may I propose another change on top of this?
>
>  I've noticed the test case is rather slow, it certainly takes much more 
> time than the average one, I've seen elapsed times of well over a minute 
> on reasonably fast hardware and occasionally a timeout midway through even 
> though the test case was otherwise progressing just fine.  I think lock 
> contention or unrelated system activity such as hardware interrupts (think 
> a busy network!) may contribute to it for systems using LL/SC loops for 
> atomicity.
>
>  So I think the default timeout that's used for really quick tests should 
> be extended a bit.  I propose a factor of 2, just not to make it too 
> excessive, at least for the beginning (maybe it'll have to be higher 
> eventually).

Do you mind if I incorporate this change on my patchset?

>
>  OK?
>
> 2014-09-03  Maciej W. Rozycki  <macro@codesourcery.com>
>
> 	gcc/testsuite/
> 	* gcc.dg/atomic/c11-atomic-exec-5.c (dg-timeout-factor): New 
> 	setting.
>
>   Maciej
>
> gcc-test-c11-atomic-exec-5-timeout-factor.diff
> Index: gcc-fsf-trunk-quilt/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c
> ===================================================================
> --- gcc-fsf-trunk-quilt.orig/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c	2014-09-02 17:34:06.718927043 +0100
> +++ gcc-fsf-trunk-quilt/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c	2014-09-03 14:51:12.958927233 +0100
> @@ -9,6 +9,7 @@
>  /* { dg-additional-options "-D_XOPEN_SOURCE=600" { target *-*-solaris2.1[0-9]* } } */
>  /* { dg-require-effective-target fenv_exceptions } */
>  /* { dg-require-effective-target pthread } */
> +/* { dg-timeout-factor 2 } */
>
>  #include <fenv.h>
>  #include <float.h>
>
Maciej W. Rozycki Sept. 15, 2014, 2:37 p.m. UTC | #3
On Thu, 4 Sep 2014, Adhemerval Zanella wrote:

> >  While at it, may I propose another change on top of this?
> >
> >  I've noticed the test case is rather slow, it certainly takes much more 
> > time than the average one, I've seen elapsed times of well over a minute 
> > on reasonably fast hardware and occasionally a timeout midway through even 
> > though the test case was otherwise progressing just fine.  I think lock 
> > contention or unrelated system activity such as hardware interrupts (think 
> > a busy network!) may contribute to it for systems using LL/SC loops for 
> > atomicity.
> >
> >  So I think the default timeout that's used for really quick tests should 
> > be extended a bit.  I propose a factor of 2, just not to make it too 
> > excessive, at least for the beginning (maybe it'll have to be higher 
> > eventually).
> 
> Do you mind if I incorporate this change on my patchset?

 I missed your e-mail previously, sorry.  Surely I don't!  Thanks.

  Maciej
Maciej W. Rozycki Oct. 20, 2014, 4:59 p.m. UTC | #4
Hi,

 I thought http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html would 
be folded into PowerPC TARGET_ATOMIC_ASSIGN_EXPAND_FENV support, but I see 
r216437 went without it.  In that case would someone please review my 
proposal as a separate change?

 Thanks,

  Maciej
David Edelsohn Oct. 21, 2014, 12:16 a.m. UTC | #5
On Mon, Oct 20, 2014 at 12:59 PM, Maciej W. Rozycki
<macro@codesourcery.com> wrote:
> Hi,
>
>  I thought http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html would
> be folded into PowerPC TARGET_ATOMIC_ASSIGN_EXPAND_FENV support, but I see
> r216437 went without it.  In that case would someone please review my
> proposal as a separate change?

The patch seems like a kludge work-around. Joseph suggested that full
support will require a newer GLIBC and detection in GCC.

Thanks, David
Joseph Myers Oct. 21, 2014, 12:26 a.m. UTC | #6
On Mon, 20 Oct 2014, David Edelsohn wrote:

> On Mon, Oct 20, 2014 at 12:59 PM, Maciej W. Rozycki
> <macro@codesourcery.com> wrote:
> > Hi,
> >
> >  I thought http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html would
> > be folded into PowerPC TARGET_ATOMIC_ASSIGN_EXPAND_FENV support, but I see
> > r216437 went without it.  In that case would someone please review my
> > proposal as a separate change?
> 
> The patch seems like a kludge work-around. Joseph suggested that full
> support will require a newer GLIBC and detection in GCC.

No, it's support for soft-float and e500 in 
TARGET_ATOMIC_ASSIGN_EXPAND_FENV that will need that (along with libgcc 
changes to make libgcc's copies of the soft-fp functions into compat 
symbols when they are available in glibc).  That's nothing to do with the 
timeout issue.
David Edelsohn Oct. 21, 2014, 2:03 a.m. UTC | #7
On Mon, Oct 20, 2014 at 8:26 PM, Joseph S. Myers
<joseph@codesourcery.com> wrote:
> On Mon, 20 Oct 2014, David Edelsohn wrote:
>
>> On Mon, Oct 20, 2014 at 12:59 PM, Maciej W. Rozycki
>> <macro@codesourcery.com> wrote:
>> > Hi,
>> >
>> >  I thought http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html would
>> > be folded into PowerPC TARGET_ATOMIC_ASSIGN_EXPAND_FENV support, but I see
>> > r216437 went without it.  In that case would someone please review my
>> > proposal as a separate change?
>>
>> The patch seems like a kludge work-around. Joseph suggested that full
>> support will require a newer GLIBC and detection in GCC.
>
> No, it's support for soft-float and e500 in
> TARGET_ATOMIC_ASSIGN_EXPAND_FENV that will need that (along with libgcc
> changes to make libgcc's copies of the soft-fp functions into compat
> symbols when they are available in glibc).  That's nothing to do with the
> timeout issue.

I can apply the patch, but I don't want to unilaterally decide to
change the timeout affecting all architectures.

Thanks, David
Maciej W. Rozycki Oct. 21, 2014, 10:47 p.m. UTC | #8
David,

> >> >  I thought http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html would
> >> > be folded into PowerPC TARGET_ATOMIC_ASSIGN_EXPAND_FENV support, but I see
> >> > r216437 went without it.  In that case would someone please review my
> >> > proposal as a separate change?
> >>
> >> The patch seems like a kludge work-around. Joseph suggested that full
> >> support will require a newer GLIBC and detection in GCC.
> >
> > No, it's support for soft-float and e500 in
> > TARGET_ATOMIC_ASSIGN_EXPAND_FENV that will need that (along with libgcc
> > changes to make libgcc's copies of the soft-fp functions into compat
> > symbols when they are available in glibc).  That's nothing to do with the
> > timeout issue.
> 
> I can apply the patch, but I don't want to unilaterally decide to
> change the timeout affecting all architectures.

 Understood, I only cc-ed you to keep you in the loop with changes somehow 
related to Power targets and stuff you have been involved with rather than 
seeking your approval.

  Maciej
Maciej W. Rozycki Nov. 14, 2014, 9 p.m. UTC | #9
Hi,

 This patch:

http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html

is still waiting, please review.

 Thanks,

  Maciej
Mike Stump Nov. 17, 2014, 9:56 a.m. UTC | #10
On Nov 14, 2014, at 1:00 PM, Maciej W. Rozycki <macro@codesourcery.com> wrote:
> http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html
> 
> is still waiting, please review.

Wait no more.

Ok.
Maciej W. Rozycki Nov. 18, 2014, 4:34 p.m. UTC | #11
On Mon, 17 Nov 2014, Mike Stump wrote:

> > http://gcc.gnu.org/ml/gcc-patches/2014-09/msg00242.html
> > 
> > is still waiting, please review.
> 
> Wait no more.
> 
> Ok.

 Applied now, thanks for your review.

  Maciej

Patch
diff mbox

Index: gcc-fsf-trunk-quilt/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c
===================================================================
--- gcc-fsf-trunk-quilt.orig/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c	2014-09-02 17:34:06.718927043 +0100
+++ gcc-fsf-trunk-quilt/gcc/testsuite/gcc.dg/atomic/c11-atomic-exec-5.c	2014-09-03 14:51:12.958927233 +0100
@@ -9,6 +9,7 @@ 
 /* { dg-additional-options "-D_XOPEN_SOURCE=600" { target *-*-solaris2.1[0-9]* } } */
 /* { dg-require-effective-target fenv_exceptions } */
 /* { dg-require-effective-target pthread } */
+/* { dg-timeout-factor 2 } */
 
 #include <fenv.h>
 #include <float.h>