Re: Some issues to resolve with XFree 4.0 yet

	Gabriel's patch is the correct way to address the problem and
should be the one which goes into the public sources.

	I do not understand, however, why the patch only includes the "=m"
constraint on regw() and not regw16().  All of the inlined functions
should have constraints which reference the actual memory address read or
written to ensure proper dependencies in optimized code.

	The problem was the unnecessary "volatile" not the memory
constraint.

David

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Hi,

Okay I will add this.  But right now the regr16 and regw16 macros are not
used at all in the r128 code in xfree86 4.0. (I put them there only for
completeness and to match the x86 versions).


Thanks,

Kevin


At 14:13 -0500 3/27/00, David Edelsohn wrote:

Gabriel's patch is the correct way to address the problem and
should be the one which goes into the public sources.

I do not understand, however, why the patch only includes the "=m"
constraint on regw() and not regw16().  All of the inlined functions
should have constraints which reference the actual memory address read or
written to ensure proper dependencies in optimized code.

The problem was the unnecessary "volatile" not the memory
constraint.

David

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Can anybody explain how method a) is different/better then method b) ?
I *lot* of drivers are using method b) so is that to say all the
developers who are using it are wrong and should change?


a) asm volatile ("stwbrx %1,%2,%3; eieio"
          : "=m" (*(volatile unsigned *)(base_addr+regindex))
          : "r" (regdata), "b" (regindex), "r" (base_addr));


b) asm volatile ("stwbrx %0,%1,%2; eieio" : : "r"(regdata), "b"
		 (regindex), "r"(base_addr) : "memory");



a) asm volatile ("lwbrx %0,%1,%2; eieio"
           : "=r"(val)
           : "b"(regindex), "r"(base_addr),
             "m" (*(volatile unsigned *)(base_addr+regindex)));

b) asm volatile ("lwbrx %0,%1,%2; eieio" : "=r"(val) : "b"(regindex),
		  "r"(base_addr));


ani


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Hi Ani,

I asked the same things a few weeks back.  David is the epxert, I am not.
I think the key is what David just wrote:

All of the inlined functions
should have constraints which reference the actual memory address read or
written to ensure proper dependencies in optimized code.

I think the memory constraint on the exact address prevents the compiler
from moving this inline code to someplace inappropriate, but David and
Gabriel could answer better.

Kevin


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Mon, 27 Mar 2000, Kevin B. Hendricks wrote:

I asked the same things a few weeks back.  David is the epxert, I am not.
I think the key is what David just wrote:

quoted

All of the inlined functions
should have constraints which reference the actual memory address read or
written to ensure proper dependencies in optimized code.

I think the memory constraint on the exact address prevents the compiler
from moving this inline code to someplace inappropriate, but David and
Gabriel could answer better.

It's not about moving inline code, it's about the compiler thinking which
in-memory variables may be clobbered by the inline code. If the constraint says
that `memory' is clobbered, the compiler cannot know which variables were
clobbered, so it will assume they were all clobbered and it will reload them
from memory into the registers instead of reusing the regoister values.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Mon, 27 Mar 2000, David Edelsohn wrote:

	Gabriel's patch is the correct way to address the problem and
should be the one which goes into the public sources.

Thanks, and sorry for the delay. I was busy on other fronts (my wife had
surgery on Monday, nothing serious however).

	I do not understand, however, why the patch only includes the "=m"
constraint on regw() and not regw16().  All of the inlined functions
should have constraints which reference the actual memory address read or
written to ensure proper dependencies in optimized code.

Well, I overlooked that. What I wanted to insist upon in my patch was that
the volatile was absolutely unnecessary.

Actually, if it were my call I would have declared the variable as
volatile unsigned  char *, which is the right type for address arithmetic
and eliminates any need for cast on byte accesses. I consider minimizing
the number or required casts as the right guideline to choose the variable
type in this case.

BTW, did anybody think of adding a __builtin_byteswap to GCC ?

This would allow the compiler to directly generate *brx instructions on
PPC by combining them with memory loads and stores. I'm aware that it
would require an additional constraint letter for indexed addressing modee
only but this is required for Altivec anyway.

And this would open opportunities for quite a lot of optimizations, for
example when setting or clearing some bits in a device register. In this
latter case (and in the given example) operands are often constants and
the compiler could generate non byte swapped load and stores and byte swap
the constants.

	Gabriel.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

At 12:45 29.03.00, Gabriel Paubert wrote:

BTW, did anybody think of adding a __builtin_byteswap to GCC ?

This would allow the compiler to directly generate *brx instructions on
PPC by combining them with memory loads and stores. I'm aware that it
would require an additional constraint letter for indexed addressing modee
only but this is required for Altivec anyway.

And this would open opportunities for quite a lot of optimizations, for
example when setting or clearing some bits in a device register. In this
latter case (and in the given example) operands are often constants and
the compiler could generate non byte swapped load and stores and byte swap
the constants.

Hmm, I was thinking about adding __attribute__((little_endian)) and
__attribute__((big_endian)) to further describe variables. This should give
us optimum optimization on various platforms. Even things like:

union {
         unsigned long little_var __attribute__((little_endian));
         unsigned long big_var __attribute__((big_endian));
}

should be possible then.

Franz.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

On Wed, 29 Mar 2000, Franz Sirl wrote:

Hmm, I was thinking about adding __attribute__((little_endian)) and
__attribute__((big_endian)) to further describe variables. This should give
us optimum optimization on various platforms. Even things like:

union {
         unsigned long little_var __attribute__((little_endian));
         unsigned long big_var __attribute__((big_endian));
}

should be possible then.

Indeed, but I was considering it as a later step. I have the feeling that
adding a builtin would be simpler and would allow to build the necessary
infrastructure for attribute support with minimal intermediate breakage
(perhaps by implementing it only on some architectures at first).

Furthermore, the subject of adding this attribute has appeared on a quite
regular basis on GCC mailing lists in the last few years and nothing has
ever been done about it AFAICT. Perhaps a different strategy through
builtin functions would get things started, that's why I'm suggesting it.

	Gabriel.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Am Wed, 29 Mar 2000 schrieb Gabriel Paubert:

On Wed, 29 Mar 2000, Franz Sirl wrote:

quoted

Hmm, I was thinking about adding __attribute__((little_endian)) and
__attribute__((big_endian)) to further describe variables. This should give
us optimum optimization on various platforms. Even things like:

union {
         unsigned long little_var __attribute__((little_endian));
         unsigned long big_var __attribute__((big_endian));
}

should be possible then.

Indeed, but I was considering it as a later step. I have the feeling that
adding a builtin would be simpler and would allow to build the necessary
infrastructure for attribute support with minimal intermediate breakage
(perhaps by implementing it only on some architectures at first).

Adding the attributes is quite simple, the part not quite clear to me yet is how
to evaluate the attribute in the backend and if we need middle-end support.
Evaluating the attribute directly in the backend mov* patterns seems
straightforward, but maybe separate reversed_mov* patterns maybe more
appropriate...

Furthermore, the subject of adding this attribute has appeared on a quite
regular basis on GCC mailing lists in the last few years and nothing has
ever been done about it AFAICT. Perhaps a different strategy through
builtin functions would get things started, that's why I'm suggesting it.

As always with GCC, if you want something done, do it yourself :-) (unless you
can pay somebody for coding).

Franz.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/