What's up of this if (likely()) bracing?

What's up of this if (likely()) bracing?

[rubisher]

Hello *,

I am not sure to have perfectly understand all subtle details of likely() &
unlikely() macros but I think there's some brace at the bad place in following
chunk:
--- arch/parisc/lib/memcpy.c.Orig	2007-10-18 15:27:30.000000000 +0000
+++ arch/parisc/lib/memcpy.c	2008-01-03 10:17:52.000000000 +0000
@@ -299,7 +299,7 @@
 
 	/* Check alignment */
 	t1 = (src ^ dst);
-	if (unlikely(t1 & (sizeof(double)-1)))
+	if (unlikely(t1 & (sizeof(double) - 1)))
 		goto unaligned_copy;
 
 	/* src and dst have same alignment. */
@@ -405,7 +405,7 @@
 
 unaligned_copy:
 	/* possibly we are aligned on a word, but not on a double... */
-	if (likely(t1 & (sizeof(unsigned int)-1)) == 0) {
+	if (likely((t1 & (sizeof(unsigned int) - 1)) == 0)) {
 		t2 = src & (sizeof(unsigned int) - 1);
 
 		if (unlikely(t2 != 0)) {
=== <> ===
First hunk is just to add some whitespace?

Otoh for the second hunk, my reading of the original stuff was that:
> likely(t1 & (sizeof(unsigned int)-1)) 
i.e. likely's macro embraced only "t1 & (sizeof(unsigned int)-1)" in place of
"(t1 & (sizeof(unsigned int)-1)) == 0".

What's your opinion?

Tia,
    r.
---
Scarlet One, ADSL 6 Mbps + Telephone, from EUR 29,95...
http://www.scarlet.be/

Re: What's up of this if (likely()) bracing?

[Grant Grundler]

On Thu, Jan 03, 2008 at 12:37:12PM +0100, rubisher wrote:
> Hello *,
> 
> I am not sure to have perfectly understand all subtle details of likely() &
> unlikely() macros but I think there's some brace at the bad place in following
> chunk:
> --- arch/parisc/lib/memcpy.c.Orig	2007-10-18 15:27:30.000000000 +0000
> +++ arch/parisc/lib/memcpy.c	2008-01-03 10:17:52.000000000 +0000
> @@ -299,7 +299,7 @@
>  
>  	/* Check alignment */
>  	t1 = (src ^ dst);
> -	if (unlikely(t1 & (sizeof(double)-1)))
> +	if (unlikely(t1 & (sizeof(double) - 1)))

Please submit separate patches for the white space clean up.

>  		goto unaligned_copy;
>  
>  	/* src and dst have same alignment. */
> @@ -405,7 +405,7 @@
>  
>  unaligned_copy:
>  	/* possibly we are aligned on a word, but not on a double... */
> -	if (likely(t1 & (sizeof(unsigned int)-1)) == 0) {
> +	if (likely((t1 & (sizeof(unsigned int) - 1)) == 0)) {
>  		t2 = src & (sizeof(unsigned int) - 1);
>  
>  		if (unlikely(t2 != 0)) {
> === <> ===
> First hunk is just to add some whitespace?
> 
> Otoh for the second hunk, my reading of the original stuff was that:
> > likely(t1 & (sizeof(unsigned int)-1)) 
> i.e. likely's macro embraced only "t1 & (sizeof(unsigned int)-1)" in place of
> "(t1 & (sizeof(unsigned int)-1)) == 0".
> 
> What's your opinion?

I think you are right. normally likely() and unlikely() are intended
to be used with boolean expressions and that's obviously not the case.

Please resubmit this change separately and add "signed-off-by" lines
so kyle can include this in the next round of parisc patches.

cheers,
grant


> 
> Tia,
>     r.
> ---
> Scarlet One, ADSL 6 Mbps + Telephone, from EUR 29,95...
> http://www.scarlet.be/
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-parisc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What's up of this if (likely()) bracing?

[rubisher]

[-- Attachment #1: Type: text/plain, Size: 7467 bytes --]

Grant Grundler wrote:
> On Thu, Jan 03, 2008 at 12:37:12PM +0100, rubisher wrote:
>> Hello *,
>>
>> I am not sure to have perfectly understand all subtle details of likely() &
>> unlikely() macros but I think there's some brace at the bad place in following
>> chunk:
>> --- arch/parisc/lib/memcpy.c.Orig	2007-10-18 15:27:30.000000000 +0000
>> +++ arch/parisc/lib/memcpy.c	2008-01-03 10:17:52.000000000 +0000
>> @@ -299,7 +299,7 @@
>>  
>>  	/* Check alignment */
>>  	t1 = (src ^ dst);
>> -	if (unlikely(t1 & (sizeof(double)-1)))
>> +	if (unlikely(t1 & (sizeof(double) - 1)))
> 
> Please submit separate patches for the white space clean up.
> 
>>  		goto unaligned_copy;
>>  
>>  	/* src and dst have same alignment. */
>> @@ -405,7 +405,7 @@
>>  
>>  unaligned_copy:
>>  	/* possibly we are aligned on a word, but not on a double... */
>> -	if (likely(t1 & (sizeof(unsigned int)-1)) == 0) {
>> +	if (likely((t1 & (sizeof(unsigned int) - 1)) == 0)) {
>>  		t2 = src & (sizeof(unsigned int) - 1);
>>  
>>  		if (unlikely(t2 != 0)) {
>> === <> ===
>> First hunk is just to add some whitespace?
>>
>> Otoh for the second hunk, my reading of the original stuff was that:
>>> likely(t1 & (sizeof(unsigned int)-1)) 
>> i.e. likely's macro embraced only "t1 & (sizeof(unsigned int)-1)" in place of
>> "(t1 & (sizeof(unsigned int)-1)) == 0".
>>
>> What's your opinion?
> 
> I think you are right. normally likely() and unlikely() are intended
> to be used with boolean expressions and that's obviously not the case.
> 
> Please resubmit this change separately and add "signed-off-by" lines
> so kyle can include this in the next round of parisc patches.
> 
> cheers,
> grant
> 
> 
Grant,

Ok here there are:
a white space cleanup/beautify patch:
Signed-off-by: Joel Soete <soete dot joel at scarlet dot be>
Index: linux-current/arch/parisc/lib/memcpy.c
===================================================================
--- linux-current.orig/arch/parisc/lib/memcpy.c	2008-01-05 13:37:04.000000000 +0000
+++ linux-current/arch/parisc/lib/memcpy.c	2008-01-05 13:55:21.000000000 +0000
@@ -21,28 +21,28 @@
   *    Copyright (C) 1991, 1997, 2003 Free Software Foundation, Inc.
   *
   * Several strategies are tried to try to get the best performance for various
- * conditions. In the optimal case, we copy 64-bytes in an unrolled loop using
+ * conditions. In the optimal case, we copy 64-bytes in an unrolled loop using
   * fp regs. This is followed by loops that copy 32- or 16-bytes at a time using
- * general registers.  Unaligned copies are handled either by aligning the
- * destination and then using shift-and-write method, or in a few cases by
+ * general registers.  Unaligned copies are handled either by aligning the
+ * destination and then using shift-and-write method, or in a few cases by
   * falling back to a byte-at-a-time copy.
   *
   * I chose to implement this in C because it is easier to maintain and debug,
   * and in my experiments it appears that the C code generated by gcc (3.3/3.4
- * at the time of writing) is fairly optimal. Unfortunately some of the
+ * at the time of writing) is fairly optimal. Unfortunately some of the
   * semantics of the copy routine (exception handling) is difficult to express
   * in C, so we have to play some tricks to get it to work.
   *
   * All the loads and stores are done via explicit asm() code in order to use
- * the right space registers.
- *
- * Testing with various alignments and buffer sizes shows that this code is
+ * the right space registers.
+ *
+ * Testing with various alignments and buffer sizes shows that this code is
   * often >10x faster than a simple byte-at-a-time copy, even for strangely
   * aligned operands. It is interesting to note that the glibc version
- * of memcpy (written in C) is actually quite fast already. This routine is
- * able to beat it by 30-40% for aligned copies because of the loop unrolling,
- * but in some cases the glibc version is still slightly faster. This lends
- * more credibility that gcc can generate very good code as long as we are
+ * of memcpy (written in C) is actually quite fast already. This routine is
+ * able to beat it by 30-40% for aligned copies because of the loop unrolling,
+ * but in some cases the glibc version is still slightly faster. This lends
+ * more credibility that gcc can generate very good code as long as we are
   * careful.
   *
   * TODO:
@@ -154,7 +154,7 @@
  #endif

  /* Copy from a not-aligned src to an aligned dst, using shifts. Handles 4 words
- * per loop.  This code is derived from glibc.
+ * per loop.  This code is derived from glibc.
   */
  static inline unsigned long copy_dstaligned(unsigned long dst, unsigned long src, unsigned long len, unsigned long o_dst, 
unsigned long o_src, unsigned long o_len)
  {
@@ -299,7 +299,7 @@

  	/* Check alignment */
  	t1 = (src ^ dst);
-	if (unlikely(t1 & (sizeof(double)-1)))
+	if (unlikely(t1 & (sizeof(double) - 1)))
  		goto unaligned_copy;

  	/* src and dst have same alignment. */
@@ -322,7 +322,7 @@

  #if 0
  	/* Copy 8 doubles at a time */
-	while (len >= 8*sizeof(double)) {
+	while (len >= 8 * sizeof(double)) {
  		register double r1, r2, r3, r4, r5, r6, r7, r8;
  		/* prefetch_src((char *)pds + L1_CACHE_BYTES); */
  		flddma(s_space, pds, r1, pmc_load_exc);
@@ -346,7 +346,7 @@
  		fstdma(d_space, r6, pdd, pmc_store_exc);
  		fstdma(d_space, r7, pdd, pmc_store_exc);
  		fstdma(d_space, r8, pdd, pmc_store_exc);
-		len -= 8*sizeof(double);
+		len -= 8 * sizeof(double);
  	}
  #endif

@@ -354,7 +354,7 @@
  	pwd = (unsigned int *)pdd;

  word_copy:
-	while (len >= 8*sizeof(unsigned int)) {
+	while (len >= 8 * sizeof(unsigned int)) {
  		register unsigned int r1,r2,r3,r4,r5,r6,r7,r8;
  		/* prefetch_src((char *)pws + L1_CACHE_BYTES); */
  		ldwma(s_space, pws, r1, pmc_load_exc);
@@ -374,7 +374,7 @@
  		stwma(d_space, r6, pwd, pmc_store_exc);
  		stwma(d_space, r7, pwd, pmc_store_exc);
  		stwma(d_space, r8, pwd, pmc_store_exc);
-		len -= 8*sizeof(unsigned int);
+		len -= 8 * sizeof(unsigned int);
  	}

  	while (len >= 4*sizeof(unsigned int)) {
@@ -387,7 +387,7 @@
  		stwma(d_space, r2, pwd, pmc_store_exc);
  		stwma(d_space, r3, pwd, pmc_store_exc);
  		stwma(d_space, r4, pwd, pmc_store_exc);
-		len -= 4*sizeof(unsigned int);
+		len -= 4 * sizeof(unsigned int);
  	}

  	pcs = (unsigned char *)pws;
@@ -438,7 +438,7 @@
  		src = (unsigned long)pcs;
  	}

-	ret = copy_dstaligned(dst, src, len / sizeof(unsigned int),
+	ret = copy_dstaligned(dst, src, len / sizeof(unsigned int),
  		o_dst, o_src, o_len);
  	if (ret)
  		return ret;
=== <> ===


and
a fixe of likely's macro usage:
Signed-off-by: Joel Soete <soete dot joel at scarlet dot be>
Index: linux-current/arch/parisc/lib/memcpy.c
===================================================================
--- linux-current.orig/arch/parisc/lib/memcpy.c	2008-01-05 13:57:26.000000000 +0000
+++ linux-current/arch/parisc/lib/memcpy.c	2008-01-05 13:58:01.000000000 +0000
@@ -405,7 +405,7 @@

  unaligned_copy:
  	/* possibly we are aligned on a word, but not on a double... */
-	if (likely(t1 & (sizeof(unsigned int)-1)) == 0) {
+	if (likely((t1 & (sizeof(unsigned int) - 1)) == 0)) {
  		t2 = src & (sizeof(unsigned int) - 1);

  		if (unlikely(t2 != 0)) {
=== <> ===

Tx again,
	r.


ps: 1/ I also attached patches' file as compressed format
     2/ I will also ask fsf to update may 'Discalimer request' to change my email address and extend it to linux tree :<)


[-- Attachment #2: beautify-memcpy.c.patch.gz --]
[-- Type: application/x-gzip, Size: 1682 bytes --]

[-- Attachment #3: fix-likely-memcpy.c.patch.gz --]
[-- Type: application/x-gzip, Size: 299 bytes --]