[Bug target/42553] New: wrong code with -O1

[Bug target/42553] New: wrong code with -O1

[debian-gcc at lists dot debian dot org]

current trunk/branches on x86_64-linux-gnu. return values with different
compilers and different optimizations:

         -O0  -O1  -O2  -O3
gcc-4.1   0    2    2    2
gcc-4.3   0    2    2    2
gcc-4.4   2    0    0    0
gcc-4.5   0    1    0    0

  Matthias

#include <stdbool.h>
#include <stdio.h>

static bool atomic_test_and_reset_bit(unsigned long *v,unsigned long bit) {
  bool res;

  __asm__ __volatile__( "btr %2,%1\n"
                        "adc $0,%0\n"
                        :"=r"(res), "=m"(*v)
                        :"r"(bit), "r"(0)) ;
  return res ? 1 : 0;
}

int main(void)
{
  unsigned long flags = 0;
  int j;

  j = atomic_test_and_reset_bit(&flags, 2);

  printf("%d\n", j);

  return 0;
}


-- 
           Summary: wrong code with -O1
           Product: gcc
           Version: 4.4.3
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: debian-gcc at lists dot debian dot org
GCC target triplet: x86_64-linux-gnu


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42553

Re: [Bug target/42553] New: wrong code with -O1

[Andrew Pinski]

Sent from my iPhone

On Dec 30, 2009, at 7:56 AM, "debian-gcc at lists dot debian dot org" <gcc-bugzilla@gcc.gnu.org 
 > wrote:

> current trunk/branches on x86_64-linux-gnu. return values with  
> different
> compilers and different optimizations:
>
>         -O0  -O1  -O2  -O3
> gcc-4.1   0    2    2    2
> gcc-4.3   0    2    2    2
> gcc-4.4   2    0    0    0
> gcc-4.5   0    1    0    0
>
>  Matthias
>
> #include <stdbool.h>
> #include <stdio.h>
>
> static bool atomic_test_and_reset_bit(unsigned long *v,unsigned long  
> bit) {
>  bool res;
>
>  __asm__ __volatile__( "btr %2,%1\n"
>                        "adc $0,%0\n"
>                        :"=r"(res), "=m"(*v)
>                        :"r"(bit), "r"(0)) ;
>  return res ? 1 : 0;

This inline-asm looks wrong. Because it says *v is always overriden.  
So if that function is inlined, flags down below does not have to be  
set before it.

> }
>
> int main(void)
> {
>  unsigned long flags = 0;
>  int j;
>
>  j = atomic_test_and_reset_bit(&flags, 2);
>
>  printf("%d\n", j);
>
>  return 0;
> }
>
>
> -- 
>           Summary: wrong code with -O1
>           Product: gcc
>           Version: 4.4.3
>            Status: UNCONFIRMED
>          Severity: normal
>          Priority: P3
>         Component: target
>        AssignedTo: unassigned at gcc dot gnu dot org
>        ReportedBy: debian-gcc at lists dot debian dot org
> GCC target triplet: x86_64-linux-gnu
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42553
>

[Bug target/42553] wrong code with -O1

[pinskia at gmail dot com]

------- Comment #1 from pinskia at gmail dot com  2009-12-30 13:14 -------
Subject: Re:   New: wrong code with -O1



Sent from my iPhone

On Dec 30, 2009, at 7:56 AM, "debian-gcc at lists dot debian dot org"
<gcc-bugzilla@gcc.gnu.org 
 > wrote:

> current trunk/branches on x86_64-linux-gnu. return values with  
> different
> compilers and different optimizations:
>
>         -O0  -O1  -O2  -O3
> gcc-4.1   0    2    2    2
> gcc-4.3   0    2    2    2
> gcc-4.4   2    0    0    0
> gcc-4.5   0    1    0    0
>
>  Matthias
>
> #include <stdbool.h>
> #include <stdio.h>
>
> static bool atomic_test_and_reset_bit(unsigned long *v,unsigned long  
> bit) {
>  bool res;
>
>  __asm__ __volatile__( "btr %2,%1\n"
>                        "adc $0,%0\n"
>                        :"=r"(res), "=m"(*v)
>                        :"r"(bit), "r"(0)) ;
>  return res ? 1 : 0;

This inline-asm looks wrong. Because it says *v is always overriden.  
So if that function is inlined, flags down below does not have to be  
set before it.

> }
>
> int main(void)
> {
>  unsigned long flags = 0;
>  int j;
>
>  j = atomic_test_and_reset_bit(&flags, 2);
>
>  printf("%d\n", j);
>
>  return 0;
> }
>
>
> -- 
>           Summary: wrong code with -O1
>           Product: gcc
>           Version: 4.4.3
>            Status: UNCONFIRMED
>          Severity: normal
>          Priority: P3
>         Component: target
>        AssignedTo: unassigned at gcc dot gnu dot org
>        ReportedBy: debian-gcc at lists dot debian dot org
> GCC target triplet: x86_64-linux-gnu
>
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42553
>


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42553

[Bug target/42553] wrong code with -O1

[pinskia at gcc dot gnu dot org]

------- Comment #2 from pinskia at gcc dot gnu dot org  2009-12-30 14:33 -------
The inline-asm is totally incorrect here is the corrected version of the
function (note res should be set):
static bool atomic_test_and_reset_bit(unsigned long *v,unsigned long bit) {
  bool res = 0;

  __asm__ __volatile__( "btr %2,%1\n"
                        "adc $0,%0\n"
                        :"+r"(res): "m"(*v)
                        ,"r"(bit), "r"(0): "flags") ;
  return res ? 1 : 0;
}


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42553

[Bug target/42553] wrong code with -O1

[ubizjak at gmail dot com]

------- Comment #3 from ubizjak at gmail dot com  2010-01-04 16:07 -------
(In reply to comment #2)
> The inline-asm is totally incorrect here ...

Actually, the asm is correct, it is just a couple of volatiles that are
missing. Please see arch/x86/include/asm/bitops.h from linux-2.6 for correct
implementation.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42553