Re: [Qemu-devel] Re: [PATCH 1/5] Fix i386 Host

[Johannes Schindelin]

Hi,

On Fri, 18 Jan 2008, Michael Matz wrote:

> On Fri, 18 Jan 2008, Johannes Schindelin wrote:
> 
> > > > >+#if DATA_SIZE == 1 || DATA_SIZE == 2
> > > > >+                  "q" (v),
> > > > >+#else
> > > > >                  "r" (v),
> > > > >+#endif
> > > > >                  "i" ((CPU_TLB_SIZE - 1) << CPU_TLB_ENTRY_BITS),
> > > > >                  "i" (TARGET_PAGE_BITS - CPU_TLB_ENTRY_BITS),
> > > > >                  "i" (TARGET_PAGE_MASK | (DATA_SIZE - 1)),
> > > > >-- snap --
> > > > >
> > > > >Michael, Alexander, what is this hunk supposed to do?
> > > 
> > > This is required to generate valid assembler code.  Without that hunk, 
> > > the 
> > > interesting parts of the asm look like so (for DATA_SIZE == 1):
> > > 
> > > asm (" ... movzbl %b1, %%edx\n ... " : : "r" (blubb), "r" (bla) );
> > 
> > Okay, but this only concerns gcc4, apparently.
> 
> No, it's nothing to do with GCC.

But apparently it has!  With gcc < 4 I did never get the error.  Which 
probably means that gcc < 4 did _not_ use ecx, and therefore it does not 
have to be pushed and popped.  Which -- judging from how commonly glue() 
is called in op.c -- could mean a performance hit.

I am all for supporting gcc > 3, but please, please not at the cost of 
having a performance hit for _existing_ users.

> > Can't we guard it with yet another "defined(GCC...)"?
> 
> Only if you want to trust your luck.  I fear I don't have gcc 3.4.2 
> lying around anywhere, so I can't really help debugging this reload 
> breakage in that GCC version.  It might help to introduce a temporary to 
> guide GCC through this problematic reload case by detaching the global 
> register variable from the asm operand.  For cases where it's no problem 
> this should be optimized away, so doesn't inhibit a performance cost.  
> What I mean is something like the below.  If someone with gcc 3.4.2 
> could test that ...

I do ask myself how gcc would optimise away instructions that are 
explicitely written in the asm() statement.  If it does so, I consider 
this a serious bug in gcc.

Ciao,
Dscho