[1/2,powerpc] add 'volatile' to asm

Message ID 1560361254-14067-2-git-send-email-pc@us.ibm.com
State Superseded
Headers show
Series
  • utilize faster method to get FPSCR
Related show

Commit Message

Paul Clarke June 12, 2019, 5:40 p.m.
From: "Paul A. Clarke" <pc@us.ibm.com>


Add 'volatile' keyword to a few asm statements, to force the compiler
to generate the instructions therein.

2019-06-12  Paul A. Clarke  <pc@us.ibm.com>

	* sysdeps/powerpc/fpu/fenv_libc.h (relax_fenv_state): Add 'volatile'.
	* sysdeps/powerpc/fpu/fpu_control.h (__FPU_MFFS): Likewise.
	(__FPU_MFFSL): Likewise.
	(_FPU_SETCW): Likewise.

v2: This fixes issues seen by Tulio in my earlier posted patch
    "[powerpc] fegetround: utilize faster method to get rounding mode"
    which was not committed.
---
 sysdeps/powerpc/fpu/fenv_libc.h | 4 ++--
 sysdeps/powerpc/fpu_control.h   | 6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)

-- 
1.8.3.1

Comments

Florian Weimer June 12, 2019, 6:55 p.m. | #1
* Paul A. Clarke:

> diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h

> index f8dd1b7..f66bf24 100644

> --- a/sysdeps/powerpc/fpu/fenv_libc.h

> +++ b/sysdeps/powerpc/fpu/fenv_libc.h

> @@ -56,9 +56,9 @@ extern const fenv_t *__fe_mask_env (void) attribute_hidden;

>  #define relax_fenv_state() \

>  	do { \

>  	   if (GLRO(dl_hwcap) & PPC_FEATURE_HAS_DFP) \

> -	     asm (".machine push; .machine \"power6\"; " \

> +	     asm volatile (".machine push; .machine \"power6\"; " \

>  		  "mtfsfi 7,0,1; .machine pop"); \

> -	   asm ("mtfsfi 7,0"); \

> +	   asm volatile ("mtfsfi 7,0"); \

>  	} while(0)


I think this change is a no-op because asm statements without inputs and
outputs (“Basic Asm”) are implicitly volatile.  Are you adding these
just for consistency?

Thanks,
Florian
Paul Clarke June 13, 2019, 6:35 p.m. | #2
On 6/12/19 1:55 PM, Florian Weimer wrote:
> * Paul A. Clarke:

>> diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h

>> index f8dd1b7..f66bf24 100644

>> --- a/sysdeps/powerpc/fpu/fenv_libc.h

>> +++ b/sysdeps/powerpc/fpu/fenv_libc.h

>> @@ -56,9 +56,9 @@ extern const fenv_t *__fe_mask_env (void) attribute_hidden;

>>  #define relax_fenv_state() \

>>  	do { \

>>  	   if (GLRO(dl_hwcap) & PPC_FEATURE_HAS_DFP) \

>> -	     asm (".machine push; .machine \"power6\"; " \

>> +	     asm volatile (".machine push; .machine \"power6\"; " \

>>  		  "mtfsfi 7,0,1; .machine pop"); \

>> -	   asm ("mtfsfi 7,0"); \

>> +	   asm volatile ("mtfsfi 7,0"); \

>>  	} while(0)

> 

> I think this change is a no-op because asm statements without inputs and

> outputs (“Basic Asm”) are implicitly volatile.  Are you adding these

> just for consistency?


Of course that's what I was doing!  ;-)

Actually, no.  What I was doing was making assumptions based on an incomplete understanding of the semantics to which you refer, but I like your explanation better.

I'm tempted to push those changes anyway, as I think there is less chance for confusion (like mine), unless there are objections.

PC

Patch

diff --git a/sysdeps/powerpc/fpu/fenv_libc.h b/sysdeps/powerpc/fpu/fenv_libc.h
index f8dd1b7..f66bf24 100644
--- a/sysdeps/powerpc/fpu/fenv_libc.h
+++ b/sysdeps/powerpc/fpu/fenv_libc.h
@@ -56,9 +56,9 @@  extern const fenv_t *__fe_mask_env (void) attribute_hidden;
 #define relax_fenv_state() \
 	do { \
 	   if (GLRO(dl_hwcap) & PPC_FEATURE_HAS_DFP) \
-	     asm (".machine push; .machine \"power6\"; " \
+	     asm volatile (".machine push; .machine \"power6\"; " \
 		  "mtfsfi 7,0,1; .machine pop"); \
-	   asm ("mtfsfi 7,0"); \
+	   asm volatile ("mtfsfi 7,0"); \
 	} while(0)
 
 /* Set/clear a particular FPSCR bit (for instance,
diff --git a/sysdeps/powerpc/fpu_control.h b/sysdeps/powerpc/fpu_control.h
index 07ccc84..0ab9331 100644
--- a/sysdeps/powerpc/fpu_control.h
+++ b/sysdeps/powerpc/fpu_control.h
@@ -67,7 +67,7 @@  typedef unsigned int fpu_control_t;
 /* Macros for accessing the hardware control word.  */
 # define __FPU_MFFS()						\
   ({register double __fr;					\
-    __asm__ ("mffs %0" : "=f" (__fr));				\
+    __asm__ __volatile__("mffs %0" : "=f" (__fr));				\
     __fr;							\
   })
 
@@ -81,7 +81,7 @@  typedef unsigned int fpu_control_t;
 #ifdef _ARCH_PWR9
 # define __FPU_MFFSL()						\
   ({register double __fr;					\
-    __asm__ ("mffsl %0" : "=f" (__fr));				\
+    __asm__ __volatile__("mffsl %0" : "=f" (__fr));				\
     __fr;							\
   })
 #else
@@ -101,7 +101,7 @@  typedef unsigned int fpu_control_t;
     __u.__ll = 0xfff80000LL << 32; /* This is a QNaN.  */	\
     __u.__ll |= (cw) & 0xffffffffLL;				\
     __fr = __u.__d;						\
-    __asm__ ("mtfsf 255,%0" : : "f" (__fr));			\
+    __asm__ __volatile__("mtfsf 255,%0" : : "f" (__fr));			\
   }
 
 /* Default control word set at startup.  */