This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] Add volatiles for x86-64 bits/mathinline.h
On Wednesday, May 02, 2012 23:32:13 Roland McGrath wrote:
> > As mentioned previously today, lrintf fails for me with gcc 4.7. The
> > resolution in bugzilla suggests to add volatiles, here's a patch to do
> > so. I will in a separate patch add checks for specific gcc versions so
> > that we use the gcc builtins.
>
> I had to read the GCC bugzilla to figure out what this was about.
> Since those asms do have output parameters, volatile doesn't seem
> like it would be required. In fact, I'm still not convinced that
> is the best fix. If it is, it certainly requires comments explaining
> why those volatiles are there.
I discussed this further in the GCC bugzilla at http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53190
Uros commented my question:
> So, how should the inline rewritten?
>
> Adding volatile is one option:
> extern __inline __attribute__ ((__always_inline__)) long int
> __attribute__ ((__nothrow__ )) lrintf (float __x)
> {
> long int __res;
> __asm __volatile__ ("cvtss2si %1, %0" : "=r" (__res) : "xm" (__x));
> return __res;
> }
>
> Since this is SSE code: Is there any way to clobber the SSE control register?
> Any better way to write the above?
With:
"The insn above doesn't clobber SSE CR, but it uses it. But there is no MXCSR reg listed in gcc ATM. I see no other way to prevent CSE or moves
of these ASMs."
> AIUI the issue is that these asms depend on the state of the fpsr, but
> the compiler doesn't know that and so eliminates a second instance
> after an fpsr change as redundant with the first. So what would be
> really right is to tell the compiler that the fpsr is an input to these
> asms and an output of the mode-changing code.
Yes, indeed - but that's not possible with GCC.
> There may be no direct way to do that if the compiler doesn't know about
> fpsr as a register name (since it's not really a register). But perhaps
> it could be done by some fakery. Declare a global variable __fpsr or
> somesuch, and then make that an input/output parameter of the asms that
> are affected by or change the fpsr. Then the compiler is free to
> eliminate the asms if their outputs are dead, or reorder them relative
> to unrelated 'asm volatile's, etc.
>
> If that is a lot of trouble and all the affected code is going away
> entirely for new-enough GCC versions, then it might not be worth the
> effort. But no matter what, thorough comments are required.
GCC 3.4 introduced the lrint builtin-functions, so I plan a followup patch to disable
them for GCC 3.4 and newer and therefore this is not worth it IMO.
Here's a patch with comments. Ok to commit ?
Andreas
2012-05-02 Andreas Jaeger <aj@suse.de>
[BZ #14053]
* sysdeps/x86_64/fpu/bits/mathinline.h (lrintf): Add __volatile to asm.
(lrint): Likewise.
(llrintf): Likewise.
(llrint): Likewise.
(rint): Likewise.
(rintf): Likewise.
(nearbyint): Likewise.
(nearbyintf): Likewise.
diff --git a/sysdeps/x86_64/fpu/bits/mathinline.h b/sysdeps/x86_64/fpu/bits/mathinline.h
index c072f16..a0c2bc1 100644
--- a/sysdeps/x86_64/fpu/bits/mathinline.h
+++ b/sysdeps/x86_64/fpu/bits/mathinline.h
@@ -79,7 +79,11 @@ __MATH_INLINE long int
__NTH (lrintf (float __x))
{
long int __res;
- __asm ("cvtss2si %1, %0" : "=r" (__res) : "xm" (__x));
+ /* Mark as volatile since the result is dependend on the state of
+ the SSE control register (the rounding mode). Otherwise GCC might
+ remove these assembler instructions since it does not know about
+ the rounding mode change. */
+ __asm __volatile__ ("cvtss2si %1, %0" : "=r" (__res) : "xm" (__x));
return __res;
}
# endif
@@ -88,7 +92,11 @@ __MATH_INLINE long int
__NTH (lrint (double __x))
{
long int __res;
- __asm ("cvtsd2si %1, %0" : "=r" (__res) : "xm" (__x));
+ /* Mark as volatile since the result is dependend on the state of
+ the SSE control register (the rounding mode). Otherwise GCC might
+ remove these assembler instructions since it does not know about
+ the rounding mode change. */
+ __asm __volatile__ ("cvtsd2si %1, %0" : "=r" (__res) : "xm" (__x));
return __res;
}
# endif
@@ -97,14 +105,22 @@ __MATH_INLINE long long int
__NTH (llrintf (float __x))
{
long long int __res;
- __asm ("cvtss2si %1, %0" : "=r" (__res) : "xm" (__x));
+ /* Mark as volatile since the result is dependend on the state of
+ the SSE control register (the rounding mode). Otherwise GCC might
+ remove these assembler instructions since it does not know about
+ the rounding mode change. */
+ __asm __volatile__ ("cvtss2si %1, %0" : "=r" (__res) : "xm" (__x));
return __res;
}
__MATH_INLINE long long int
__NTH (llrint (double __x))
{
long long int __res;
- __asm ("cvtsd2si %1, %0" : "=r" (__res) : "xm" (__x));
+ /* Mark as volatile since the result is dependend on the state of
+ the SSE control register (the rounding mode). Otherwise GCC might
+ remove these assembler instructions since it does not know about
+ the rounding mode change. */
+ __asm __volatile__ ("cvtsd2si %1, %0" : "=r" (__res) : "xm" (__x));
return __res;
}
# endif
@@ -176,14 +192,14 @@ __MATH_INLINE double
__NTH (rint (double __x))
{
double __res;
- __asm ("roundsd $4, %1, %0" : "=x" (__res) : "xm" (__x));
+ __asm __volatile__ ("roundsd $4, %1, %0" : "=x" (__res) : "xm" (__x));
return __res;
}
__MATH_INLINE float
__NTH (rintf (float __x))
{
float __res;
- __asm ("roundss $4, %1, %0" : "=x" (__res) : "xm" (__x));
+ __asm __volatile__ ("roundss $4, %1, %0" : "=x" (__res) : "xm" (__x));
return __res;
}
@@ -193,14 +209,22 @@ __MATH_INLINE double
__NTH (nearbyint (double __x))
{
double __res;
- __asm ("roundsd $0xc, %1, %0" : "=x" (__res) : "xm" (__x));
+ /* Mark as volatile since the result is dependend on the state of
+ the SSE control register (the rounding mode). Otherwise GCC might
+ remove these assembler instructions since it does not know about
+ the rounding mode change. */
+ __asm __volatile__ ("roundsd $0xc, %1, %0" : "=x" (__res) : "xm" (__x));
return __res;
}
__MATH_INLINE float
__NTH (nearbyintf (float __x))
{
float __res;
- __asm ("roundss $0xc, %1, %0" : "=x" (__res) : "xm" (__x));
+ /* Mark as volatile since the result is dependend on the state of
+ the SSE control register (the rounding mode). Otherwise GCC might
+ remove these assembler instructions since it does not know about
+ the rounding mode change. */
+ __asm __volatile__ ("roundss $0xc, %1, %0" : "=x" (__res) : "xm" (__x));
return __res;
}
# endif
--
Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126