This is the mail archive of the cygwin-patches mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]