memory: of_memory.c: remove unnecessary initialization

PATCH In review Cong Ding 4 participants 8 messages Dec 4, 2012 – Dec 24, 2012 Memory controller drivers

lore

Share & tools

lore

Cong Ding

author

2012-12-04 11:28 UTC #0 lore raw

PATCH 1/1 memory: of_memory.c: remove unnecessary initialization

the initialization of variable ret is unnecessary, we can remove it while save one time "or" operation.

Signed-off-by Cong Ding <[email protected]>

---
 drivers/memory/of_memory.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/memory/of_memory.c +2 -2

index 6007435..c59a946 100644
@@ -30,14 +30,14 @@
 const struct lpddr2_min_tck *of_get_min_tck(struct device_node *np,
 		struct device *dev)
 {
-	int			ret = 0;
+	int			ret;
 	struct lpddr2_min_tck	*min;
 
 	min = devm_kzalloc(dev, sizeof(*min), GFP_KERNEL);
 	if (!min)
 		goto default_min_tck;
 
-	ret |= of_property_read_u32(np, "tRPab-min-tck", &min->tRPab);
+	ret = of_property_read_u32(np, "tRPab-min-tck", &min->tRPab);
 	ret |= of_property_read_u32(np, "tRCD-min-tck", &min->tRCD);
 	ret |= of_property_read_u32(np, "tWR-min-tck", &min->tWR);
 	ret |= of_property_read_u32(np, "tRASmin-min-tck", &min->tRASmin);

Signature · 1 line

1.7.4.5

Santosh Shilimkar

Acked-by on v1 1/1

2012-12-04 11:44 UTC #1 lore raw

Quoted 5 lines · Cong Ding

the initialization of variable ret is unnecessary, we can remove it while save one time "or" operation.
Signed-off-by Cong Ding <[email protected]>
---

Looks ok.

Acked-by Santosh Shilimkar<[email protected]>

Grant Likely

2012-12-04 13:55 UTC #2 lore raw

Quoted 6 lines · Santosh Shilimkar

Quoted 5 lines · Cong Ding
the initialization of variable ret is unnecessary, we can remove it while save
one time "or" operation.

Quoted 6 lines

Quoted 2 lines
Signed-off-by Cong Ding <[email protected]>
---
Looks ok.
Acked-by Santosh Shilimkar<[email protected]>

Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong.

--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Santosh Shilimkar

2012-12-04 14:46 UTC #3 lore raw

Quoted 11 lines · Grant Likely

Quoted 9 lines · Santosh Shilimkar
Quoted 7 lines · Cong Ding
the initialization of variable ret is unnecessary, we can remove it while save
one time "or" operation.
Signed-off-by Cong Ding <[email protected]>
---

Quoted 7 lines

Quoted 3 lines
Looks ok.
Acked-by Santosh Shilimkar<[email protected]>
Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong.

The patch was removing an additional operation and hence i didn't contest it. I agree with your comment though.

Regards
Santosh

Li, Zhen-Hua

2012-12-05 07:26 UTC #4 lore raw

Infact, your patch does remove an orl operation, but add a new "move" operation.

You can test such two functions:

int func1(int rm1, int rm2){
        int i = 0;
        i |= rm1;
        i |= rm2;
}

and

int func(int rm1, int rm2){
        int i;
        i = rm1;
        i |= rm2;
}

Use gcc to compile them to assemble with "-S" operation, and you will find it.

Quoted 37 lines · Santosh Shilimkar

Quoted 16 lines · Grant Likely
Quoted 13 lines · Santosh Shilimkar
Quoted 9 lines · Cong Ding
the initialization of variable ret is unnecessary, we can remove it while
save
one time "or" operation.
Signed-off-by Cong Ding <[email protected]>
---
Quoted 7 lines
Quoted 3 lines
Looks ok.
Acked-by Santosh Shilimkar<[email protected]>
Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong.
The patch was removing an additional operation and hence i didn't contest it. I agree with your comment though.
Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Li, Zhen-Hua

2012-12-05 08:04 UTC #5 lore raw

And if you use -O2 as gcc option, you may find it does nothing. They are using the same assemble language.

Quoted 58 lines · Zhen-Hua

Infact, your patch does remove an orl operation, but add a new "move" operation.
You can test such two functions:
int func1(int rm1, int rm2){
        int i = 0;
        i |= rm1;
        i |= rm2;
}
and
int func(int rm1, int rm2){
        int i;
        i = rm1;
        i |= rm2;
}
Use gcc to compile them to assemble with "-S" operation, and you will find it.
Quoted 37 lines · Santosh Shilimkar
Quoted 16 lines · Grant Likely
Quoted 13 lines · Santosh Shilimkar
Quoted 9 lines · Cong Ding
the initialization of variable ret is unnecessary, we can remove it while
save
one time "or" operation.
Signed-off-by Cong Ding <[email protected]>
---
Quoted 7 lines
Quoted 3 lines
Looks ok.
Acked-by Santosh Shilimkar<[email protected]>
Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong.
The patch was removing an additional operation and hence i didn't contest it. I agree with your comment though.
Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Cong Ding

author

2012-12-05 10:06 UTC #6 lore raw

In reply to Li, Zhen-Hua

Quoted 11 lines · Zhen-Hua

Infact, your patch does remove an orl operation, but add a new "move" operation.
You can test such two functions:
int func1(int rm1, int rm2){
        int i = 0;
        i |= rm1;
        i |= rm2;
}
and

Quoted 7 lines

int func(int rm1, int rm2){
        int i;
        i = rm1;
        i |= rm2;
}
Use gcc to compile them to assemble with "-S" operation, and you will find it.

you are wrong. if we use O0 parameter in gcc, it really reduces an "OR" operation; and you are correct if we use O2 in gcc, the assemble code is the same. you can refer to the following screen snapshot.

But we should not rely on compilers, right? But in this situation, this simple optimization should be done by any compiler, so it doesn't matter we patch it or not.

[ding@GNU ~]$ gcc --version
gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-2)
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

[ding@GNU ~]$ cat main1.c
#include<stdio.h>

int foo(int arg1, int arg2) {
        int ret = 0;
        ret |= arg1;
        ret |= arg2;
        return ret;
}

int main(int argc, char **argv) {
        int o = foo(57, 89);
        printf(value is %d.n, o);
}

[ding@GNU ~]$ cat main2.c
#include<stdio.h>

int foo(int arg1, int arg2) {
        int ret;
        ret = arg1;
        ret |= arg2;
        return ret;
}

int main(int argc, char **argv) {
        int o = foo(57, 89);
        printf(value is %d.n, o);
}

[ding@GNU ~]$ gcc -S main1.c -o main1.s
[ding@GNU ~]$ gcc -S main2.c -o main2.s
[ding@GNU ~]$ diff -up main1.s main2.s

main2.s 2012-12-05 09:23:25.742997827 +0000 renamed from main1.s 2012-12-05 09:23:18.487007457 +0000 +2 -3

@@ -1,4 +1,4 @@
-       .file   main1.c
+       .file   main2.c
        .text
        .globl  foo
        .type   foo, @function

@@ -12,9 +12,8 @@ foo:
        .cfi_def_cfa_register 6
        movl    %edi, -20(%rbp)
        movl    %esi, -24(%rbp)
-       movl    -bash, -4(%rbp)
        movl    -20(%rbp), %eax
-       orl     %eax, -4(%rbp)
+       movl    %eax, -4(%rbp)
        movl    -24(%rbp), %eax
        orl     %eax, -4(%rbp)
        movl    -4(%rbp), %eax

[ding@GNU ~]$ gcc -S -O2 main1.c -o main1O2.s
[ding@GNU ~]$ gcc -S -O2 main2.c -o main2O2.s
[ding@GNU ~]$ diff -up main1O2.s main2O2.s

main2O2.s renamed from main1O2.s +1 -1

@@ -1,4 +1,4 @@
-	.file	"main1.c"
+	.file	"main2.c"
 	.text
 	.p2align 4,,15
 	.globl	foo

[ding@GNU ~]$

Quoted 39 lines

Quoted 37 lines · Santosh Shilimkar
Quoted 16 lines · Grant Likely
Quoted 13 lines · Santosh Shilimkar
Quoted 9 lines · Cong Ding
the initialization of variable ret is unnecessary, we can remove it while
save
one time "or" operation.
Signed-off-by Cong Ding <[email protected]>
---
Quoted 7 lines
Quoted 3 lines
Looks ok.
Acked-by Santosh Shilimkar<[email protected]>
Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong.
The patch was removing an additional operation and hence i didn't contest it. I agree with your comment though.
Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Li, Zhen-Hua

2012-12-24 09:00 UTC #7 lore raw

Yes, I think I was wrong and you are right. I did test again and now it is clear.

Quoted 135 lines · Cong Ding

Quoted 11 lines · Zhen-Hua
Infact, your patch does remove an orl operation, but add a new "move" operation.
You can test such two functions:
int func1(int rm1, int rm2){
        int i = 0;
        i |= rm1;
        i |= rm2;
}
and
Quoted 7 lines
int func(int rm1, int rm2){
        int i;
        i = rm1;
        i |= rm2;
}
Use gcc to compile them to assemble with "-S" operation, and you will find it.
you are wrong. if we use O0 parameter in gcc, it really reduces an "OR" operation; and you are correct if we use O2 in gcc, the assemble code is the same. you can refer to the following screen snapshot.
But we should not rely on compilers, right? But in this situation, this simple optimization should be done by any compiler, so it doesn't matter we patch it or not.
[ding@GNU ~]$ gcc --version
gcc (GCC) 4.6.2 20111027 (Red Hat 4.6.2-2)
Copyright (C) 2011 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
[ding@GNU ~]$ cat main1.c
#include<stdio.h>
int foo(int arg1, int arg2) {
        int ret = 0;
        ret |= arg1;
        ret |= arg2;
        return ret;
}
int main(int argc, char **argv) {
        int o = foo(57, 89);
        printf(value is %d.n, o);
}
[ding@GNU ~]$ cat main2.c
#include<stdio.h>
int foo(int arg1, int arg2) {
        int ret;
        ret = arg1;
        ret |= arg2;
        return ret;
}
int main(int argc, char **argv) {
        int o = foo(57, 89);
        printf(value is %d.n, o);
}
[ding@GNU ~]$ gcc -S main1.c -o main1.s
[ding@GNU ~]$ gcc -S main2.c -o main2.s
[ding@GNU ~]$ diff -up main1.s main2.s
main2.s 2012-12-05 09:23:25.742997827 +0000 renamed from main1.s 2012-12-05 09:23:18.487007457 +0000 +2 -3
@@ -1,4 +1,4 @@
-       .file   main1.c
+       .file   main2.c
        .text
        .globl  foo
        .type   foo, @function
@@ -12,9 +12,8 @@ foo:
        .cfi_def_cfa_register 6
        movl    %edi, -20(%rbp)
        movl    %esi, -24(%rbp)
-       movl    -bash, -4(%rbp)
        movl    -20(%rbp), %eax
-       orl     %eax, -4(%rbp)
+       movl    %eax, -4(%rbp)
        movl    -24(%rbp), %eax
        orl     %eax, -4(%rbp)
        movl    -4(%rbp), %eax
[ding@GNU ~]$ gcc -S -O2 main1.c -o main1O2.s
[ding@GNU ~]$ gcc -S -O2 main2.c -o main2O2.s
[ding@GNU ~]$ diff -up main1O2.s main2O2.s
main2O2.s 2012-12-05 09:24:22.590911258 +0000 renamed from main1O2.s 2012-12-05 09:24:12.718928945 +0000 +1 -1
@@ -1,4 +1,4 @@
-       .file   "main1.c"
+       .file   "main2.c"
        .text
        .p2align 4,,15
        .globl  foo
[ding@GNU ~]$
Quoted 39 lines
Quoted 37 lines · Santosh Shilimkar
Quoted 16 lines · Grant Likely
Quoted 13 lines · Santosh Shilimkar
Quoted 9 lines · Cong Ding
the initialization of variable ret is unnecessary, we can remove it while
save
one time "or" operation.
Signed-off-by Cong Ding <[email protected]>
---
Quoted 7 lines
Quoted 3 lines
Looks ok.
Acked-by Santosh Shilimkar<[email protected]>
Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong.
The patch was removing an additional operation and hence i didn't contest it. I agree with your comment though.
Regards
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/