Re: Bug in haifa scheduler ?

[Gabriel Paubert <paubert at iram dot es>]

On Sat, 27 Jun 1998, Franz Sirl wrote:

> I think this is not enough. The eieio (Old MacDonald had a farm, I love this
> instruction :-))  ) instruction (if we want to avoid the general memory 
> clobber for in/out's) has to be integrated into the in/out volatile asm,
> otherwise it can again be reordered around the in/out's by the compiler, which
> is not what we want.

I disagree, a series of 2 asm volatile should not be reordered
relative to each other, this simply does not make sense. And if it
does happen, it is a compiler bug. 

BTW: I made a detailed comparison of the assembly output of platinum.c
with both versions of eieio. I have not been able to see any difference in
the order of I/O port accesses and eieio instructions. The generated code
is different simply because one variable (plat_regs) is repeatedly
reloaded because of the memory clobber.  Note that I also tried to change
the eieio macro to be a simple asm("eieio"), without any colon to specify
parameters. It generates still a different assembly code but once again I
fail to see any significant difference. I might have to look closer,
however... 

Anyway the platinum.c code basically does not use any of the in/out
macros, but I have found 2 places where the code is at the very least
doubtful. One is in read_platinum_sense, all writes to the regs[23].r
are followed by eieio and a delay except the last one. This is however
a minor point IMHO. 

The other one is the code which determines the frame buffer size, this
code is extremely fragile. Some memory mapped as write-through is written
to and later read from to know if the address effectively corresponds to
physical storage.  This will fail is the data happens for any reason to be
in the cache (L1 or L2), the eieio which is beteen the read and write
accesses could better be replaced by a true memory barrier and the cache
lines should be invalidated just in case. This does not explain the
observed problems however.

Finally, why is plat_regs declared volatile and cmap_regs is not. This
might be a problem: there are places with an out_8(cmap_regs->addr) 
followed by a test of another cmap_regs like: 

	out_8(&cmap_regs->addr, 3+32);
	if (cmap_regs->d2 == 2) {
		STORE_D2(7, clksel[0]);
		STORE_D2(8, clksel[1]);
		STORE_D2(3, 3);


since cmap_regs is a normal pointer (not to volatile), the compiler
may reorder the read before the out_8 if it seems fit for scheduling 
purposes. Adding the memory clobber to eieio prevents this, but IMHO
declaring cmap_regs as a pointer to volatile should have the same effect.
 
> 
> So it should look something like that:
> 
> outw():
> asm volatile("eieio; sthbrx %1,0,%2" : "=m" (*port): "r" (data), "r" (port));
> 
> inw():
> asm volatile("eieio; lhbrx %0,0,%1" : "=r" (data): "r" (port), "m" (*port));
> 

Right now we do the eieio after, but it should not matter. 

> And for eieio() uses indepedent of the in/out inlines we should keep the
> general memory clobber.
> 
> eieio():
> asm volatile("eieio" : : : "memory");
> 

Once again I'm not so sure. 

	Gabriel.