Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-07-30 Thread Benjamin Herrenschmidt 
On Thu, 2010-07-08 at 16:08 +1000, Benjamin Herrenschmidt wrote:
 On Fri, 2010-06-25 at 11:56 +0200, Jakub Jelinek wrote:
 
   static inline void sync(void)
  diff --git a/arch/powerpc/include/asm/atomic.h 
  b/arch/powerpc/include/asm/atomic.h
  index b8f152e..288d8b2 100644
  --- a/arch/powerpc/include/asm/atomic.h
  +++ b/arch/powerpc/include/asm/atomic.h
  @@ -19,14 +19,14 @@ static __inline__ int atomic_read(const atomic_t *v)
   {
  int t;
   
  -   __asm__ __volatile__(lwz%U1%X1 %0,%1 : =r(t) : m(v-counter));
  +   __asm__ __volatile__(lwz%U1%X1 %0,%1 : =r(t) : m(v-counter));
   
  return t;
   }
   
 
 This gives me:
 
 /home/benh/linux-powerpc-test/arch/powerpc/kernel/time.c: In function 
 ‘timer_interrupt’:
 /home/benh/linux-powerpc-test/arch/powerpc/include/asm/atomic.h:22: error: 
 ‘asm’ operand has impossible constraints
 make[2]: *** [arch/powerpc/kernel/time.o] Error 1
 
 $ gcc --version
 gcc (Debian 4.4.4-1) 4.4.4

Ping :-)

Do that mean that 4.4.4 doesn't understand your new constraints or are
we doing something incorrect ?

Cheers,
Ben.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-07-30 Thread Jakub Jelinek 
On Fri, Jul 30, 2010 at 05:04:46PM +1000, Benjamin Herrenschmidt wrote:
 On Thu, 2010-07-08 at 16:08 +1000, Benjamin Herrenschmidt wrote:
  On Fri, 2010-06-25 at 11:56 +0200, Jakub Jelinek wrote:
  
static inline void sync(void)
   diff --git a/arch/powerpc/include/asm/atomic.h 
   b/arch/powerpc/include/asm/atomic.h
   index b8f152e..288d8b2 100644
   --- a/arch/powerpc/include/asm/atomic.h
   +++ b/arch/powerpc/include/asm/atomic.h
   @@ -19,14 +19,14 @@ static __inline__ int atomic_read(const atomic_t *v)
{
 int t;

   - __asm__ __volatile__(lwz%U1%X1 %0,%1 : =r(t) : m(v-counter));
   + __asm__ __volatile__(lwz%U1%X1 %0,%1 : =r(t) : m(v-counter));

 return t;
}

  
  This gives me:
  
  /home/benh/linux-powerpc-test/arch/powerpc/kernel/time.c: In function 
  ‘timer_interrupt’:
  /home/benh/linux-powerpc-test/arch/powerpc/include/asm/atomic.h:22: error: 
  ‘asm’ operand has impossible constraints
  make[2]: *** [arch/powerpc/kernel/time.o] Error 1
  
  $ gcc --version
  gcc (Debian 4.4.4-1) 4.4.4
 
 Ping :-)
 
 Do that mean that 4.4.4 doesn't understand your new constraints or are
 we doing something incorrect ?

The constraints weren't new, so in theory everything would work fine.
Except because  and  were so rarely used on many targets before,
there were backend bugs on PowerPC and SPARC at least related to that.
See
http://gcc.gnu.org/PR44707
http://gcc.gnu.org/PR44701
http://gcc.gnu.org/PR44492

So, in short, I'm afraid m needs to be used only for GCC 4.6+
(and, vendors which backported the inline-asm handling changes
to their older gcc would need to adjust for their gccs too).
When m isn't used, it just leads to potential code pessimization
in inline-asms that are prepared for handling side-effects.

Jakub
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-07-30 Thread Benjamin Herrenschmidt 
On Fri, 2010-07-30 at 09:19 +0200, Jakub Jelinek wrote:
 So, in short, I'm afraid m needs to be used only for GCC 4.6+
 (and, vendors which backported the inline-asm handling changes
 to their older gcc would need to adjust for their gccs too).
 When m isn't used, it just leads to potential code pessimization
 in inline-asms that are prepared for handling side-effects. 

Ok, so we'll need some kind of macro to fixup those constraints ...

Just to make sure I understand things properly, if we don't change them,
the code will still be correct with 4.6 but sub-optimal, right ?

Cheers,
Ben.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-07-08 Thread Benjamin Herrenschmidt 
On Fri, 2010-06-25 at 11:56 +0200, Jakub Jelinek wrote:

  static inline void sync(void)
 diff --git a/arch/powerpc/include/asm/atomic.h 
 b/arch/powerpc/include/asm/atomic.h
 index b8f152e..288d8b2 100644
 --- a/arch/powerpc/include/asm/atomic.h
 +++ b/arch/powerpc/include/asm/atomic.h
 @@ -19,14 +19,14 @@ static __inline__ int atomic_read(const atomic_t *v)
  {
   int t;
  
 - __asm__ __volatile__(lwz%U1%X1 %0,%1 : =r(t) : m(v-counter));
 + __asm__ __volatile__(lwz%U1%X1 %0,%1 : =r(t) : m(v-counter));
  
   return t;
  }
  

This gives me:

/home/benh/linux-powerpc-test/arch/powerpc/kernel/time.c: In function 
‘timer_interrupt’:
/home/benh/linux-powerpc-test/arch/powerpc/include/asm/atomic.h:22: error: 
‘asm’ operand has impossible constraints
make[2]: *** [arch/powerpc/kernel/time.o] Error 1

$ gcc --version
gcc (Debian 4.4.4-1) 4.4.4


Cheers,
Ben.


___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-06-30 Thread Benjamin Herrenschmidt 
On Fri, 2010-06-25 at 13:18 +0200, Jakub Jelinek wrote:
 On Fri, Jun 25, 2010 at 01:08:23PM +0200, Segher Boessenkool wrote:
   - stw%U0%X0 %L2,%1
   - : =m (*ptep), =m (*((unsigned char *)ptep+4))
   + stw%U1%X1 %L2,%1
   + : =m (*ptep), =m (*((unsigned char *)ptep+4))
 : r (pte) : memory);
  
  This still isn't correct -- the constraint says that a byte
  is written, but the insn changes a word.  Probably should just
  be ptep[1] ?

Oops, almost forgot about this. Are you guys shooting a new patch or do
you want me to do it ?

Cheers,
Ben.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-06-30 Thread Segher Boessenkool 

-   stw%U0%X0 %L2,%1
-   : =m (*ptep), =m (*((unsigned char *)ptep+4))
+   stw%U1%X1 %L2,%1
+   : =m (*ptep), =m (*((unsigned char *)ptep+4))
: r (pte) : memory);


This still isn't correct -- the constraint says that a byte
is written, but the insn changes a word.  Probably should just
be ptep[1] ?


Oops, almost forgot about this. Are you guys shooting a new patch  
or do

you want me to do it ?


It's really an independent fix.  Just take Jakub's patch, I'll do one
on top of it?


Segher

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-06-25 Thread Segher Boessenkool 
 - stw%U0%X0 %L2,%1
 - : =m (*ptep), =m (*((unsigned char *)ptep+4))
 + stw%U1%X1 %L2,%1
 + : =m (*ptep), =m (*((unsigned char *)ptep+4))
   : r (pte) : memory);

This still isn't correct -- the constraint says that a byte
is written, but the insn changes a word.  Probably should just
be ptep[1] ?


Segher

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

Re: [PATCH] Adjust arch/powerpc inline asms for recent gcc change

2010-06-25 Thread Jakub Jelinek 
On Fri, Jun 25, 2010 at 01:08:23PM +0200, Segher Boessenkool wrote:
  -   stw%U0%X0 %L2,%1
  -   : =m (*ptep), =m (*((unsigned char *)ptep+4))
  +   stw%U1%X1 %L2,%1
  +   : =m (*ptep), =m (*((unsigned char *)ptep+4))
  : r (pte) : memory);
 
 This still isn't correct -- the constraint says that a byte
 is written, but the insn changes a word.  Probably should just
 be ptep[1] ?

Yeah.

Jakub
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev