Skip to contentSkip to conversation

misplaced parenthesis?

PATCH In review Roel Kluin 4 participants 7 messages May 15, 2009 – Jun 26, 2009 PA-RISC architecture

lore
Share & tools
lore

Roel Kluin

author
#0 lore raw

PATCH 1/1 misplaced parenthesis?

Fix misplaced parenthesis.

---
I think this is what was intended? Note that this patch may affect profiling.

arch/parisc/lib/memcpy.c +1 -1
index bbda909..30207b0 100644
@@ -405,7 +405,7 @@ byte_copy:
 
 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)) {
#1 lore raw

This is a bit confusingly written, but your patch does not appear to be correct.

== has higher precedence than &, so you are basically changing it to:
if (likely(t1 & 1))

it really should be

- if (likely(t1 & (sizeof(unsigned int)-1)) == 0) {
+ if (likely((t1 & (sizeof(unsigned int)-1)) == 0)) {

randolph

Quoted 25 lines · Roel Kluin

Fix misplaced parenthesis.

---
I think this is what was intended? Note that this patch may affect profiling.

arch/parisc/lib/memcpy.c +1 -2
index bbda909..30207b0 100644
@@ -405,7 +405,7 @@ byte_copy:
 
 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)) {
--

To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw

Does gcc produce different code for the three variations?

Matt

On Sat, May 16, 2009 at 12:14 AM, Randolph Chung <[email protected]> w= rote:

Quoted 1 line

This is a bit confusingly written, but your patch does not appear to =

be

Quoted 3 lines

correct.

=3D=3D has higher precedence than &, so you are basically changing it=

to:

Quoted 19 lines

if (likely(t1 & 1))

it really should be

- =A0 =A0 =A0 if (likely(t1 & (sizeof(unsigned int)-1)) =3D=3D 0) {
+ =A0 =A0 =A0 if (likely((t1 & (sizeof(unsigned int)-1)) =3D=3D 0)) {

randolph

Quoted 8 lines · Roel Kluin

Fix misplaced parenthesis.

---
I think this is what was intended? Note that this patch may affect profiling.

Quoted 7 lines
Quoted 7 lines
arch/parisc/lib/memcpy.c +0 -0
index bbda909..30207b0 100644
@@ -405,7 +405,7 @@ byte_copy:

=A0=A0unaligned_copy:
=A0 =A0 =A0 =A0/* possibly we are aligned on a word, but not on a do=

uble... */

Quoted 3 lines
Quoted 3 lines

- =A0 =A0 =A0 if (likely(t1 & (sizeof(unsigned int)-1)) =3D=3D 0) {
+ =A0 =A0 =A0 if (likely(t1 & (sizeof(unsigned int)-1) =3D=3D 0)) { =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0t2 =3D src & (sizeof(unsigned int) - =

1);

Quoted 3 lines
Quoted 3 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (unlikely(t2 !=3D 0)) { --
To unsubscribe from this list: send the line "unsubscribe linux-pari=

sc" in

Quoted 6 lines
Quoted 3 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-paris=

c" in

Quoted 3 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw

The wrong version of course produces different code....

the original and the corrected one may produce different code depending on how the compiler schedules things.

randolph

Quoted 57 lines · Matt Turner

Does gcc produce different code for the three variations?

Matt

Quoted 36 lines · Randolph Chung

This is a bit confusingly written, but your patch does not appear to be correct.

== has higher precedence than &, so you are basically changing it to:
if (likely(t1 & 1))

it really should be

(quoted diff) +1 -1
-       if (likely(t1 & (sizeof(unsigned int)-1)) == 0) {
+       if (likely((t1 & (sizeof(unsigned int)-1)) == 0)) {

randolph

Quoted 20 lines · Roel Kluin

Fix misplaced parenthesis.

---
I think this is what was intended? Note that this patch may affect profiling.

arch/parisc/lib/memcpy.c +1 -2
index bbda909..30207b0 100644
@@ -405,7 +405,7 @@ byte_copy:
  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)) {
--

To unsubscribe from this list: send the line "unsubscribe linux-parisc" in

Quoted 10 lines
Quoted 4 lines

the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw

Here's an updated patch:

arch/parisc/lib/memcpy.c +1 -1
index 2d68431..827e535 100644
@@ -405,7 +405,7 @@ byte_copy:
 
 unaligned_copy:
        /* possibly we are aligned on a word, but not on a double... */
-       if (likely(t1 & (sizeof(unsigned int)-1)) =3D=3D 0) {
+       if (likely((t1 & (sizeof(unsigned int)-1)) =3D=3D 0)) {
                t2 =3D src & (sizeof(unsigned int) - 1);
 
                if (unlikely(t2 !=3D 0)) {

On Mon, May 18, 2009 at 9:42 AM, Randolph Chung<[email protected]> wro= te:

Quoted 3 lines

The wrong version of course produces different code....

the original and the corrected one may produce different code dependi=

ng on

Quoted 16 lines

how the compiler schedules things.

randolph

Quoted 11 lines · Matt Turner

Does gcc produce different code for the three variations?

Matt

On Sat, May 16, 2009 at 12:14 AM, Randolph Chung <[email protected]=

Quoted 2 lines · wrote:

This is a bit confusingly written, but your patch does not appear t=

o be

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

correct.

=3D=3D has higher precedence than &, so you are basically changing =

it to:

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines

if (likely(t1 & 1))

it really should be

- =A0 =A0 =A0 if (likely(t1 & (sizeof(unsigned int)-1)) =3D=3D 0) {
+ =A0 =A0 =A0 if (likely((t1 & (sizeof(unsigned int)-1)) =3D=3D 0))=

{

Quoted 14 lines
Quoted 14 lines
Quoted 14 lines

randolph

Quoted 8 lines · Roel Kluin

Fix misplaced parenthesis.

---
I think this is what was intended? Note that this patch may affect profiling.

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
arch/parisc/lib/memcpy.c +0 -0
index bbda909..30207b0 100644
@@ -405,7 +405,7 @@ byte_copy:

=A0unaligned_copy:
=A0 =A0 =A0 /* possibly we are aligned on a word, but not on a dou=

ble... */

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

- =A0 =A0 =A0 if (likely(t1 & (sizeof(unsigned int)-1)) =3D=3D 0) =

{

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

+ =A0 =A0 =A0 if (likely(t1 & (sizeof(unsigned int)-1) =3D=3D 0)) =

{

Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line

=A0 =A0 =A0 =A0 =A0 =A0 =A0 t2 =3D src & (sizeof(unsigned int) - 1=

);

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (unlikely(t2 !=3D 0)) { --
To unsubscribe from this list: send the line "unsubscribe linux-pa=

risc"

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

in
the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.ht=

ml

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-par=

isc"

Quoted 3 lines
Quoted 3 lines
Quoted 3 lines

in
the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.htm=

l

Quoted 5 lines
Quoted 5 lines
Quoted 2 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-pari=

sc" in

Quoted 5 lines
Quoted 3 lines

the body of a message to [email protected]
More majordomo info at =A0http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-parisc"=
in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 11 lines · On Tue, 23 Jun 2009 22:53:26 +0800 Randolph Chung <[email protected]> wrote:

Here's an updated patch:

arch/parisc/lib/memcpy.c +0 -0
index 2d68431..827e535 100644

arch/parisc/lib/memcpy.c byte_copy: · line 411

Quoted 8 lines
arch/parisc/lib/memcpy.c +1 -1
@@ -405,7 +405,7 @@ byte_copy:
 
 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)) {

I think I'll delete this email. If someone has a patch which they think should be applied, please resend it with a changelog.

#6 lore raw

Kyle has merged this into the parisc tree.

thanks
randolph

Quoted 29 lines · Andrew Morton
Quoted 12 lines · On Tue, 23 Jun 2009 22:53:26 +0800 Randolph Chung <[email protected]> wrote:

Here's an updated patch:

arch/parisc/lib/memcpy.c +0 -0
index 2d68431..827e535 100644

arch/parisc/lib/memcpy.c byte_copy: · line 411

Quoted 8 lines
arch/parisc/lib/memcpy.c +1 -1
@@ -405,7 +405,7 @@ byte_copy:
 
 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)) {

I think I'll delete this email. If someone has a patch which they think should be applied, please resend it with a changelog.