[PATCH] bitops: remove condition code clobber for CLZ

[PATCH] bitops: remove condition code clobber for CLZ

[Rabin Vincent]

The CLZ instruction does not alter the condition flags, so remove the
"cc" clobber from the inline asm for fls().

Signed-off-by: Rabin Vincent <rabin@rab.in>
---
 arch/arm/include/asm/bitops.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
index 338ff19..7b1bb2b 100644
--- a/arch/arm/include/asm/bitops.h
+++ b/arch/arm/include/asm/bitops.h
@@ -285,7 +285,7 @@ static inline int fls(int x)
 	if (__builtin_constant_p(x))
 	       return constant_fls(x);
 
-	asm("clz\t%0, %1" : "=r" (ret) : "r" (x) : "cc");
+	asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
        	ret = 32 - ret;
 	return ret;
 }
-- 
1.7.2.3

[PATCH] bitops: remove condition code clobber for CLZ

[Nicolas Pitre]

On Tue, 11 Jan 2011, Rabin Vincent wrote:

> The CLZ instruction does not alter the condition flags, so remove the
> "cc" clobber from the inline asm for fls().
> 
> Signed-off-by: Rabin Vincent <rabin@rab.in>

Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>


> ---
>  arch/arm/include/asm/bitops.h |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/arm/include/asm/bitops.h b/arch/arm/include/asm/bitops.h
> index 338ff19..7b1bb2b 100644
> --- a/arch/arm/include/asm/bitops.h
> +++ b/arch/arm/include/asm/bitops.h
> @@ -285,7 +285,7 @@ static inline int fls(int x)
>  	if (__builtin_constant_p(x))
>  	       return constant_fls(x);
>  
> -	asm("clz\t%0, %1" : "=r" (ret) : "r" (x) : "cc");
> +	asm("clz\t%0, %1" : "=r" (ret) : "r" (x));
>         	ret = 32 - ret;
>  	return ret;
>  }
> -- 
> 1.7.2.3
> 

[PATCH] bitops: remove condition code clobber for CLZ

[Russell King - ARM Linux]

On Tue, Jan 11, 2011 at 11:12:05PM +0530, Rabin Vincent wrote:
> The CLZ instruction does not alter the condition flags, so remove the
> "cc" clobber from the inline asm for fls().

Do you have any evidence that this changes anything, or is it just
subjective?

[PATCH] bitops: remove condition code clobber for CLZ

[Nicolas Pitre]

On Tue, 11 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 11, 2011 at 11:12:05PM +0530, Rabin Vincent wrote:
> > The CLZ instruction does not alter the condition flags, so remove the
> > "cc" clobber from the inline asm for fls().
> 
> Do you have any evidence that this changes anything, or is it just
> subjective?

This probably doesn't change anything, as gcc has been presuming that 
inline asms do clobber the condition code for years now, in order to 
prevent issues caused by a lack of %? appended to instructions in order 
to conditionally execute them otherwise.  So this would only make the 
code self consistent.


Nicolas

[PATCH] bitops: remove condition code clobber for CLZ

[Russell King - ARM Linux]

On Tue, Jan 11, 2011 at 01:51:19PM -0500, Nicolas Pitre wrote:
> On Tue, 11 Jan 2011, Russell King - ARM Linux wrote:
> 
> > On Tue, Jan 11, 2011 at 11:12:05PM +0530, Rabin Vincent wrote:
> > > The CLZ instruction does not alter the condition flags, so remove the
> > > "cc" clobber from the inline asm for fls().
> > 
> > Do you have any evidence that this changes anything, or is it just
> > subjective?
> 
> This probably doesn't change anything, as gcc has been presuming that 
> inline asms do clobber the condition code for years now, in order to 
> prevent issues caused by a lack of %? appended to instructions in order 
> to conditionally execute them otherwise.  So this would only make the 
> code self consistent.

So given that Linus complains about churn from ARM, what's the
justification to apply a patch which has no effect what so ever?

[PATCH] bitops: remove condition code clobber for CLZ

[Nicolas Pitre]

On Tue, 11 Jan 2011, Russell King - ARM Linux wrote:

> On Tue, Jan 11, 2011 at 01:51:19PM -0500, Nicolas Pitre wrote:
> > On Tue, 11 Jan 2011, Russell King - ARM Linux wrote:
> > 
> > > On Tue, Jan 11, 2011 at 11:12:05PM +0530, Rabin Vincent wrote:
> > > > The CLZ instruction does not alter the condition flags, so remove the
> > > > "cc" clobber from the inline asm for fls().
> > > 
> > > Do you have any evidence that this changes anything, or is it just
> > > subjective?
> > 
> > This probably doesn't change anything, as gcc has been presuming that 
> > inline asms do clobber the condition code for years now, in order to 
> > prevent issues caused by a lack of %? appended to instructions in order 
> > to conditionally execute them otherwise.  So this would only make the 
> > code self consistent.
> 
> So given that Linus complains about churn from ARM, what's the
> justification to apply a patch which has no effect what so ever?

Code self documentation.

If we know the inline asm is actually clobbering the condition code, 
then it makes sense to list cc in the clobber list.  And conversely as 
well, for the benefit of those reading that code.  Having cc in the 
clobber list for some inline asm that does not actually clobber the 
condition code is just confusing.


Nicolas

[PATCH] bitops: remove condition code clobber for CLZ

[Rabin Vincent]

On Wed, Jan 12, 2011 at 00:21, Nicolas Pitre <nico@fluxnic.net> wrote:
> On Tue, 11 Jan 2011, Russell King - ARM Linux wrote:
>> On Tue, Jan 11, 2011 at 11:12:05PM +0530, Rabin Vincent wrote:
>> > The CLZ instruction does not alter the condition flags, so remove the
>> > "cc" clobber from the inline asm for fls().
>>
>> Do you have any evidence that this changes anything, or is it just
>> subjective?
>
> This probably doesn't change anything, as gcc has been presuming that
> inline asms do clobber the condition code for years now, in order to
> prevent issues caused by a lack of %? appended to instructions in order
> to conditionally execute them otherwise.

FWIW, note that I am able to see GCC using the condition code clobber
information.  For example, for this (artificial) test code:

static inline int fls(int x)
{
	int ret;

#ifdef CLOBBER
	asm("clz %0,%1" : "=r" (ret) : "r" (x): "cc");
#else
	asm("clz %0,%1" : "=r" (ret) : "r" (x));
#endif
	return 32 - ret;
}

int f(int b)
{
	int x = fls(b);
	return b > 100 ? x : x + 10;
}

I get (GCC 4.5.1, at -O2):

--- clobber.s   2011-01-12 19:41:36.000000000 +0530
+++ noclobber.s 2011-01-12 19:41:36.000000000 +0530
@@ -1,12 +1,12 @@

-clobber.o:     file format elf32-littlearm
+noclobber.o:     file format elf32-littlearm


 Disassembly of section .text:

 00000000 <f>:
-   0:  e16f3f10        clz     r3, r0
-   4:  e3500064        cmp     r0, #100        ; 0x64
-   8:  e2630020        rsb     r0, r3, #32
+   0:  e3500064        cmp     r0, #100        ; 0x64
+   4:  e16f0f10        clz     r0, r0
+   8:  e2600020        rsb     r0, r0, #32
    c:  d280000a        addle   r0, r0, #10
   10:  e12fff1e        bx      lr

[PATCH] bitops: remove condition code clobber for CLZ

[Russell King - ARM Linux]

On Wed, Jan 12, 2011 at 07:48:10PM +0530, Rabin Vincent wrote:
> On Wed, Jan 12, 2011 at 00:21, Nicolas Pitre <nico@fluxnic.net> wrote:
> > On Tue, 11 Jan 2011, Russell King - ARM Linux wrote:
> >> On Tue, Jan 11, 2011 at 11:12:05PM +0530, Rabin Vincent wrote:
> >> > The CLZ instruction does not alter the condition flags, so remove the
> >> > "cc" clobber from the inline asm for fls().
> >>
> >> Do you have any evidence that this changes anything, or is it just
> >> subjective?
> >
> > This probably doesn't change anything, as gcc has been presuming that
> > inline asms do clobber the condition code for years now, in order to
> > prevent issues caused by a lack of %? appended to instructions in order
> > to conditionally execute them otherwise.
> 
> FWIW, note that I am able to see GCC using the condition code clobber
> information.  For example, for this (artificial) test code:

Ok, better code generation is a reason to apply this patch (not that
the example particularly shows that the new order was better - but it
can potentially be if there's loads around.)