This is the mail archive of the
cygwin-patches
mailing list for the Cygwin project.
Re: [PATCH] Re: 1.7 winbase.h (ilockcmpexch) compile error
Corinna Vinschen wrote:
> On Jul 12 21:52, Christopher Faylor wrote:
>> There is a subtle difference in the generated code if you do this:
>>
>> --- winbase.h 7 Jul 2009 21:41:43 -0000 1.16
>> +++ winbase.h 13 Jul 2009 01:46:17 -0000
>> @@ -56,7 +56,7 @@
>> {
>> return
>> ({
>> - register long ret __asm ("%eax");
>> + register long ret;
>> __asm __volatile ("lock cmpxchgl %2, %1"
>> : "=a" (ret), "=m" (*t)
>> : "r" (v), "m" (*t), "0" (c)
>>
>> but does it really matter? This causes the esi register to be used
>> rather than the edx register.
>>
>> with _asm ("%eax")
>> 160e: 8b 5d 08 mov 0x8(%ebp),%ebx
>> 1611: 8d 53 08 lea 0x8(%ebx),%edx
>> 1614: f0 0f b1 0a lock cmpxchg %ecx,(%edx)
>> 1618: 85 c0 test %eax,%eax
>> 161a: 74 37 je 1653 <pthread_mutex::_trylock(pthread*)+0x53>
>>
>> without
>> 1616: 8b 5d 08 mov 0x8(%ebp),%ebx
>> 1619: 8d 73 08 lea 0x8(%ebx),%esi
>> 161c: f0 0f b1 0e lock cmpxchg %ecx,(%esi)
>> 1620: 85 c0 test %eax,%eax
>> 1622: 74 44 je 1668 <pthread_mutex::_trylock(pthread*)+0x68>
>>
>>
>> And, more crucially, it compiles with gcc 3.4.
>>
>> Should I check this variation in?
>
> The affected operations have nothing to do with %eax. Why does the
> compiler change the usage of some entirely unrelated register? This
> looks suspicious.
I did explore this, and I didn't like the look of it. I found more diffs in
the rest of the file, not just a couple of changed registers, but some
functions spilled more registers to the stack and had larger frames and longer
prologues as a result. I wrote:
> (I experimented briefly with removing the register asm from the source and
> building it with gcc-4.3.2, and the results were disappointing; we actually
> got worse register allocation, resulting in some functions having larger stack
> frames and more registers saved/restored, so I guess the RA can still benefit
> from the extra hint.)
For examining these changes, I compiled the files using --save-temps and ran
a quick "sed -i -e's/^L[^ ]*:'/LLABEL/' on the generated .s file to reduce the
amount of extraneous diffs caused by renumbering of local labels when I
compared the before-and-after versions.
>> Is there a chance that using the esi register obliterates data in the
>> calling function?
It should be properly saved and restored. I didn't see anything that I
could nail down as incorrect code gen, but it was notably worse which I
figured was a consequence of how a lot of the optimisers don't try and look
inside asms. So I figured the register allocator really does benefit from
being given the hint about where to put 'ret'.
I guess the best thing would be to add a __GNUC_ version check:
{
return
({
register long ret
#if __GNUC__ >= 4
/* Register asms trip a reload bug in gcc-3.4.4. */
__asm ("%eax");
#endif
__asm __volatile ("lock cmpxchgl %2, %1"
: "=a" (ret), "=m" (*t)
: "r" (v), "m" (*t), "0" (c)
... but I'd also suggest having a good look at the generated assembly and
making sure it does correctly save and restore the used registers and doesn't
do anything else too bizarre.
cheers,
DaveK