PATCH: Re: Inefficient ia64 system call implementation in glibc

PATCH: Re: Inefficient ia64 system call implementation in glibc

[H. J. Lu]

On Mon, Sep 22, 2003 at 04:21:23PM -0700, Richard Henderson wrote:
> On Mon, Sep 22, 2003 at 12:39:18PM -0700, H. J. Lu wrote:
> > Can I get char * from char [300]?
> 
> x+0 would work in this case; I'd guess it'd work for most of the
> cases that syscalls need to handle.
> 

This patch works for me.


H.J.
---
2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>

	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
	__typeof ((outX) + 0) instead of long.
	(LOAD_ARGS_2): Likewise.
	(LOAD_ARGS_3): Likewise.
	(LOAD_ARGS_4): Likewise.
	(LOAD_ARGS_5): Likewise.
	(LOAD_ARGS_6): Likewise.

--- sysdeps/unix/sysv/linux/ia64/sysdep.h.inline	2003-08-21 07:05:30.000000000 -0700
+++ sysdeps/unix/sysv/linux/ia64/sysdep.h	2003-09-23 11:04:02.000000000 -0700
@@ -191,23 +191,23 @@
 #define INTERNAL_SYSCALL_ERRNO(val, err)	(val)
 
 #define LOAD_ARGS_0()   do { } while (0)
-#define LOAD_ARGS_1(out0)				\
-  register long _out0 asm ("out0") = (long) (out0);	\
+#define LOAD_ARGS_1(out0)					\
+  register __typeof ((out0) + 0) _out0 asm ("out0") = (out0);	\
   LOAD_ARGS_0 ()
-#define LOAD_ARGS_2(out0, out1)				\
-  register long _out1 asm ("out1") = (long) (out1);	\
+#define LOAD_ARGS_2(out0, out1)					\
+  register __typeof ((out1) + 0) _out1 asm ("out1") = (out1);	\
   LOAD_ARGS_1 (out0)
-#define LOAD_ARGS_3(out0, out1, out2)			\
-  register long _out2 asm ("out2") = (long) (out2);	\
+#define LOAD_ARGS_3(out0, out1, out2)				\
+  register __typeof ((out2) + 0) _out2 asm ("out2") = (out2);	\
   LOAD_ARGS_2 (out0, out1)
-#define LOAD_ARGS_4(out0, out1, out2, out3)		\
-  register long _out3 asm ("out3") = (long) (out3);	\
+#define LOAD_ARGS_4(out0, out1, out2, out3)			\
+  register __typeof ((out3) + 0) _out3 asm ("out3") = (out3);	\
   LOAD_ARGS_3 (out0, out1, out2)
-#define LOAD_ARGS_5(out0, out1, out2, out3, out4)	\
-  register long _out4 asm ("out4") = (long) (out4);	\
+#define LOAD_ARGS_5(out0, out1, out2, out3, out4)		\
+  register __typeof ((out4) + 0) _out4 asm ("out4") = (out4);	\
   LOAD_ARGS_4 (out0, out1, out2, out3)
-#define LOAD_ARGS_6(out0, out1, out2, out3, out4, out5)	\
-  register long _out5 asm ("out5") = (long) (out5);	\
+#define LOAD_ARGS_6(out0, out1, out2, out3, out4, out5)		\
+  register __typeof ((out5) + 0) _out5 asm ("out5") = (out5);	\
   LOAD_ARGS_5 (out0, out1, out2, out3, out4)
 
 #define ASM_OUTARGS_0

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[Jakub Jelinek]

On Wed, Sep 24, 2003 at 10:36:21AM +0200, Andreas Schwab wrote:
> >> x+0 would work in this case; I'd guess it'd work for most of the
> >> cases that syscalls need to handle.
> >> 
> >
> > This patch works for me.
> >
> >
> > H.J.
> > ---
> > 2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>
> >
> > 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
> > 	__typeof ((outX) + 0) instead of long.
> 
> Hopefully we don't have any occurences of LOAD_ARGS_n(...,0,..) where the
> kernel expects long.

I think that's very risky and could mean hard to find bugs.
I don't think the 0-1 cycle speedup is not worth the troubles.

	Jakub

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[Andreas Schwab]

"H. J. Lu" <hjl@lucon.org> writes:

> On Mon, Sep 22, 2003 at 04:21:23PM -0700, Richard Henderson wrote:
>> On Mon, Sep 22, 2003 at 12:39:18PM -0700, H. J. Lu wrote:
>> > Can I get char * from char [300]?
>> 
>> x+0 would work in this case; I'd guess it'd work for most of the
>> cases that syscalls need to handle.
>> 
>
> This patch works for me.
>
>
> H.J.
> ---
> 2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>
>
> 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
> 	__typeof ((outX) + 0) instead of long.

Hopefully we don't have any occurences of LOAD_ARGS_n(...,0,..) where the
kernel expects long.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[H. J. Lu]

On Wed, Sep 24, 2003 at 10:36:21AM +0200, Andreas Schwab wrote:
> "H. J. Lu" <hjl@lucon.org> writes:
> 
> > On Mon, Sep 22, 2003 at 04:21:23PM -0700, Richard Henderson wrote:
> >> On Mon, Sep 22, 2003 at 12:39:18PM -0700, H. J. Lu wrote:
> >> > Can I get char * from char [300]?
> >> 
> >> x+0 would work in this case; I'd guess it'd work for most of the
> >> cases that syscalls need to handle.
> >> 
> >
> > This patch works for me.
> >
> >
> > H.J.
> > ---
> > 2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>
> >
> > 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
> > 	__typeof ((outX) + 0) instead of long.
> 
> Hopefully we don't have any occurences of LOAD_ARGS_n(...,0,..) where the
> kernel expects long.
> 

I don't think it will be the problem. Compiler will do

	addl outN = constant, r0

to pass a constant to the function, regardless what the type is. Am I
correct?


H.J.

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[Andreas Schwab]

"H. J. Lu" <hjl@lucon.org> writes:

> On Wed, Sep 24, 2003 at 10:36:21AM +0200, Andreas Schwab wrote:
>> "H. J. Lu" <hjl@lucon.org> writes:
>> 
>> > On Mon, Sep 22, 2003 at 04:21:23PM -0700, Richard Henderson wrote:
>> >> On Mon, Sep 22, 2003 at 12:39:18PM -0700, H. J. Lu wrote:
>> >> > Can I get char * from char [300]?
>> >> 
>> >> x+0 would work in this case; I'd guess it'd work for most of the
>> >> cases that syscalls need to handle.
>> >> 
>> >
>> > This patch works for me.
>> >
>> >
>> > H.J.
>> > ---
>> > 2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>
>> >
>> > 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
>> > 	__typeof ((outX) + 0) instead of long.
>> 
>> Hopefully we don't have any occurences of LOAD_ARGS_n(...,0,..) where the
>> kernel expects long.
>> 
>
> I don't think it will be the problem. Compiler will do
>
> 	addl outN = constant, r0
>
> to pass a constant to the function, regardless what the type is. Am I
> correct?

It doesn't have to be a constant, it can be an arbitrary expression of the
wrong (narrower) type.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux AG, Deutschherrnstr. 15-19, D-90429 Nürnberg
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[Jim Wilson]

On Wed, 2003-09-24 at 11:56, H. J. Lu wrote:
> I don't think it will be the problem. Compiler will do
> 	addl outN = constant, r0
> to pass a constant to the function, regardless what the type is. Am I
> correct?

No.

The compiler will emit RTL like this
	(set (reg:SI outN) (const_int constant))
which will translate to a
	addl outN = constant, r0
instruction.

However, if you are optimizing, then all we are guaranteed is that a
value equivalent to an SImode constant will end up in the register.  If
you have something like
  if (i = 0)
     sub (0);
and i has type int, and i happens to be already in the right register
for the first argument to sub, then the compiler will optimize away the
load of zero as redundant.  The if statement uses cmp4 which only looks
at the low order 4 bytes of i, and hence any value could be in the upper
4 bytes.  This is OK per the ABI, which says that only the low 4 bytes
of an integer argument are valid.

I have seen reports of this happening for unnamed arguments to stdarg
functions.  There was even one report of this from David Mosberger a
long time ago.  If you are passing a constant zero to a function that
requires long, and you don't have a prototype that forces the conversion
to long, then you must specify 0L to get correct results.

I'm not subscribed to the glibc list, so I think my message will bounce
from there.
-- 
Jim Wilson, GNU Tools Support, http://www.SpecifixInc.com

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[H. J. Lu]

On Wed, Sep 24, 2003 at 02:12:38PM -0700, Jim Wilson wrote:
> On Wed, 2003-09-24 at 11:56, H. J. Lu wrote:
> > I don't think it will be the problem. Compiler will do
> > 	addl outN = constant, r0
> > to pass a constant to the function, regardless what the type is. Am I
> > correct?
> 
> No.
> 
> The compiler will emit RTL like this
> 	(set (reg:SI outN) (const_int constant))
> which will translate to a
> 	addl outN = constant, r0
> instruction.
> 
> However, if you are optimizing, then all we are guaranteed is that a
> value equivalent to an SImode constant will end up in the register.  If
> you have something like
>   if (i = 0)
>      sub (0);
> and i has type int, and i happens to be already in the right register
> for the first argument to sub, then the compiler will optimize away the
> load of zero as redundant.  The if statement uses cmp4 which only looks
> at the low order 4 bytes of i, and hence any value could be in the upper
> 4 bytes.  This is OK per the ABI, which says that only the low 4 bytes
> of an integer argument are valid.
> 

My patch only applies to LOAD_ARGS_##nr in

#define INTERNAL_SYSCALL(name, err, nr, args...)                \
  ({                                                            \
    register long _r8 asm ("r8");                               \
    register long _r10 asm ("r10");                             \
    register long _r15 asm ("r15") = __NR_##name;               \
    long _retval;                                               \
    LOAD_ARGS_##nr (args);                                      \
    __asm __volatile (BREAK_INSN (__BREAK_SYSCALL)              \
                      : "=r" (_r8), "=r" (_r10), "=r" (_r15)    \
                        ASM_OUTARGS_##nr                        \
                      : "2" (_r15) ASM_ARGS_##nr                \
                      : "memory" ASM_CLOBBERS_##nr);            \
    _retval = _r8;                                              \
    err = _r10;                                                 \
    _retval; })

I don't think it is an issue here. The current one is

#define LOAD_ARGS_1(out0)                               \
  register long _out0 asm ("out0") = (long) (out0);     \
  LOAD_ARGS_0 ()

I don't believe it is any better than

#define LOAD_ARGS_1(out0)                                      \
  register __typeof ((out0) + 0) _out0 asm ("out0") = (out0);  \
 
as far as function prototype is concerned. If there is a mismatch,
long can have problems too.


H.J.

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[H. J. Lu]

On Wed, Sep 24, 2003 at 10:36:48PM +0200, Andreas Schwab wrote:
> "H. J. Lu" <hjl@lucon.org> writes:
> 
> > On Wed, Sep 24, 2003 at 10:36:21AM +0200, Andreas Schwab wrote:
> >> "H. J. Lu" <hjl@lucon.org> writes:
> >> 
> >> > On Mon, Sep 22, 2003 at 04:21:23PM -0700, Richard Henderson wrote:
> >> >> On Mon, Sep 22, 2003 at 12:39:18PM -0700, H. J. Lu wrote:
> >> >> > Can I get char * from char [300]?
> >> >> 
> >> >> x+0 would work in this case; I'd guess it'd work for most of the
> >> >> cases that syscalls need to handle.
> >> >> 
> >> >
> >> > This patch works for me.
> >> >
> >> >
> >> > H.J.
> >> > ---
> >> > 2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>
> >> >
> >> > 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
> >> > 	__typeof ((outX) + 0) instead of long.
> >> 
> >> Hopefully we don't have any occurences of LOAD_ARGS_n(...,0,..) where the
> >> kernel expects long.
> >> 
> >
> > I don't think it will be the problem. Compiler will do
> >
> > 	addl outN = constant, r0
> >
> > to pass a constant to the function, regardless what the type is. Am I
> > correct?
> 
> It doesn't have to be a constant, it can be an arbitrary expression of the
> wrong (narrower) type.
> 

Those macros are internal to libc. We can fix all those wrong types
if there are any. My initial test shows no problem. I will install
it on my IPF machine to give it a try.


H.J.

Re: PATCH: Re: Inefficient ia64 system call implementation in glibc

[H. J. Lu]

On Wed, Sep 24, 2003 at 09:43:19AM +0200, Jakub Jelinek wrote:
> On Wed, Sep 24, 2003 at 10:36:21AM +0200, Andreas Schwab wrote:
> > >> x+0 would work in this case; I'd guess it'd work for most of the
> > >> cases that syscalls need to handle.
> > >> 
> > >
> > > This patch works for me.
> > >
> > >
> > > H.J.
> > > ---
> > > 2003-09-22  H.J. Lu  <hongjiu.lu@intel.com>
> > >
> > > 	* sysdeps/unix/sysv/linux/ia64/sysdep.h (LOAD_ARGS_1): Use
> > > 	__typeof ((outX) + 0) instead of long.
> > 
> > Hopefully we don't have any occurences of LOAD_ARGS_n(...,0,..) where the
> > kernel expects long.
> 
> I think that's very risky and could mean hard to find bugs.
> I don't think the 0-1 cycle speedup is not worth the troubles.

I don't think it is any riskier than using long. Those are internal
to libc. We don't have to worry about any users will misuse them. We
just have to make sure we pass the right types of arguments to kernel
in glibc. I don't think it is too much to ask.


H.J.