* [PATCH] memory: of_memory.c: remove unnecessary initialization @ 2012-12-04 11:26 Cong Ding 2012-12-04 11:44 ` Santosh Shilimkar 0 siblings, 1 reply; 8+ messages in thread From: Cong Ding @ 2012-12-04 11:26 UTC (permalink / raw) To: Benoit Cousson, Aneesh V, Santosh Shilimkar, Greg Kroah-Hartman, Grant Likely, linux-kernel Cc: Cong Ding the initialization of variable ret is unnecessary, we can remove it while save one time "or" operation. Signed-off-by: Cong Ding <dinggnu@gmail.com> --- drivers/memory/of_memory.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/memory/of_memory.c b/drivers/memory/of_memory.c index 6007435..c59a946 100644 --- a/drivers/memory/of_memory.c +++ b/drivers/memory/of_memory.c @@ -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); -- 1.7.4.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-04 11:26 [PATCH] memory: of_memory.c: remove unnecessary initialization Cong Ding @ 2012-12-04 11:44 ` Santosh Shilimkar 2012-12-04 13:55 ` Grant Likely 0 siblings, 1 reply; 8+ messages in thread From: Santosh Shilimkar @ 2012-12-04 11:44 UTC (permalink / raw) To: Cong Ding Cc: Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Grant Likely, linux-kernel On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: > the initialization of variable ret is unnecessary, we can remove it while save > one time "or" operation. > > Signed-off-by: Cong Ding <dinggnu@gmail.com> > --- Looks ok. Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-04 11:44 ` Santosh Shilimkar @ 2012-12-04 13:55 ` Grant Likely 2012-12-04 14:46 ` Santosh Shilimkar 0 siblings, 1 reply; 8+ messages in thread From: Grant Likely @ 2012-12-04 13:55 UTC (permalink / raw) To: Santosh Shilimkar Cc: Cong Ding, Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Linux Kernel Mailing List On Tue, Dec 4, 2012 at 11:44 AM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: >> >> the initialization of variable ret is unnecessary, we can remove it while >> save >> one time "or" operation. >> >> Signed-off-by: Cong Ding <dinggnu@gmail.com> >> --- > > Looks ok. > Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> > Thanks for the patch, but I don't think it matters enough to apply it. The existing code isn't wrong. g. -- Grant Likely, B.Sc., P.Eng. Secret Lab Technologies Ltd. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-04 13:55 ` Grant Likely @ 2012-12-04 14:46 ` Santosh Shilimkar 2012-12-05 7:26 ` Li, Zhen-Hua 0 siblings, 1 reply; 8+ messages in thread From: Santosh Shilimkar @ 2012-12-04 14:46 UTC (permalink / raw) To: Grant Likely Cc: Cong Ding, Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Linux Kernel Mailing List On Tuesday 04 December 2012 07:25 PM, Grant Likely wrote: > On Tue, Dec 4, 2012 at 11:44 AM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: >>> >>> the initialization of variable ret is unnecessary, we can remove it while >>> save >>> one time "or" operation. >>> >>> Signed-off-by: Cong Ding <dinggnu@gmail.com> >>> --- >> >> Looks ok. >> Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> > > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-04 14:46 ` Santosh Shilimkar @ 2012-12-05 7:26 ` Li, Zhen-Hua 2012-12-05 7:41 ` Li, Zhen-Hua 2012-12-05 10:06 ` Cong Ding 0 siblings, 2 replies; 8+ messages in thread From: Li, Zhen-Hua @ 2012-12-05 7:26 UTC (permalink / raw) To: Santosh Shilimkar Cc: Grant Likely, Cong Ding, Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Linux Kernel Mailing List 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. On Tue, Dec 4, 2012 at 10:46 PM, Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > On Tuesday 04 December 2012 07:25 PM, Grant Likely wrote: >> >> On Tue, Dec 4, 2012 at 11:44 AM, Santosh Shilimkar >> <santosh.shilimkar@ti.com> wrote: >>> >>> On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: >>>> >>>> >>>> the initialization of variable ret is unnecessary, we can remove it >>>> while >>>> save >>>> one time "or" operation. >>>> >>>> Signed-off-by: Cong Ding <dinggnu@gmail.com> >>>> --- >>> >>> >>> Looks ok. >>> Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>> >> >> 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 majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-05 7:26 ` Li, Zhen-Hua @ 2012-12-05 7:41 ` Li, Zhen-Hua 2012-12-05 10:06 ` Cong Ding 1 sibling, 0 replies; 8+ messages in thread From: Li, Zhen-Hua @ 2012-12-05 7:41 UTC (permalink / raw) To: Santosh Shilimkar Cc: Grant Likely, Cong Ding, Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Linux Kernel Mailing List And if you use -O2 as gcc option, you may find it does nothing. They are using the same assemble language. On Wed, Dec 5, 2012 at 3:26 PM, Li, Zhen-Hua <lizhenhua.dev@gmail.com> wrote: > 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. > > On Tue, Dec 4, 2012 at 10:46 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: >> On Tuesday 04 December 2012 07:25 PM, Grant Likely wrote: >>> >>> On Tue, Dec 4, 2012 at 11:44 AM, Santosh Shilimkar >>> <santosh.shilimkar@ti.com> wrote: >>>> >>>> On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: >>>>> >>>>> >>>>> the initialization of variable ret is unnecessary, we can remove it >>>>> while >>>>> save >>>>> one time "or" operation. >>>>> >>>>> Signed-off-by: Cong Ding <dinggnu@gmail.com> >>>>> --- >>>> >>>> >>>> Looks ok. >>>> Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >>>> >>> >>> 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 majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-05 7:26 ` Li, Zhen-Hua 2012-12-05 7:41 ` Li, Zhen-Hua @ 2012-12-05 10:06 ` Cong Ding 2012-12-24 9:00 ` Li, Zhen-Hua 1 sibling, 1 reply; 8+ messages in thread From: Cong Ding @ 2012-12-05 10:06 UTC (permalink / raw) To: Li, Zhen-Hua Cc: Santosh Shilimkar, Grant Likely, Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Linux Kernel Mailing List On Wed, Dec 05, 2012 at 03:26:36PM +0800, Li, Zhen-Hua wrote: > 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. 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 --- main1.s 2012-12-05 09:23:18.487007457 +0000 +++ main2.s 2012-12-05 09:23:25.742997827 +0000 @@ -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 --- main1O2.s 2012-12-05 09:24:12.718928945 +0000 +++ main2O2.s 2012-12-05 09:24:22.590911258 +0000 @@ -1,4 +1,4 @@ - .file "main1.c" + .file "main2.c" .text .p2align 4,,15 .globl foo [ding@GNU ~]$ > On Tue, Dec 4, 2012 at 10:46 PM, Santosh Shilimkar > <santosh.shilimkar@ti.com> wrote: > > On Tuesday 04 December 2012 07:25 PM, Grant Likely wrote: > >> > >> On Tue, Dec 4, 2012 at 11:44 AM, Santosh Shilimkar > >> <santosh.shilimkar@ti.com> wrote: > >>> > >>> On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: > >>>> > >>>> > >>>> the initialization of variable ret is unnecessary, we can remove it > >>>> while > >>>> save > >>>> one time "or" operation. > >>>> > >>>> Signed-off-by: Cong Ding <dinggnu@gmail.com> > >>>> --- > >>> > >>> > >>> Looks ok. > >>> Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> > >>> > >> > >> 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 majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] memory: of_memory.c: remove unnecessary initialization 2012-12-05 10:06 ` Cong Ding @ 2012-12-24 9:00 ` Li, Zhen-Hua 0 siblings, 0 replies; 8+ messages in thread From: Li, Zhen-Hua @ 2012-12-24 9:00 UTC (permalink / raw) To: Cong Ding Cc: Santosh Shilimkar, Grant Likely, Benoit Cousson, Aneesh V, Greg Kroah-Hartman, Linux Kernel Mailing List Yes, I think I was wrong and you are right. I did test again and now it is clear. On Wed, Dec 5, 2012 at 6:06 PM, Cong Ding <dinggnu@gmail.com> wrote: > On Wed, Dec 05, 2012 at 03:26:36PM +0800, Li, Zhen-Hua wrote: >> 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. > 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 > --- main1.s 2012-12-05 09:23:18.487007457 +0000 > +++ main2.s 2012-12-05 09:23:25.742997827 +0000 > @@ -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 > --- main1O2.s 2012-12-05 09:24:12.718928945 +0000 > +++ main2O2.s 2012-12-05 09:24:22.590911258 +0000 > @@ -1,4 +1,4 @@ > - .file "main1.c" > + .file "main2.c" > .text > .p2align 4,,15 > .globl foo > [ding@GNU ~]$ > >> On Tue, Dec 4, 2012 at 10:46 PM, Santosh Shilimkar >> <santosh.shilimkar@ti.com> wrote: >> > On Tuesday 04 December 2012 07:25 PM, Grant Likely wrote: >> >> >> >> On Tue, Dec 4, 2012 at 11:44 AM, Santosh Shilimkar >> >> <santosh.shilimkar@ti.com> wrote: >> >>> >> >>> On Tuesday 04 December 2012 04:56 PM, Cong Ding wrote: >> >>>> >> >>>> >> >>>> the initialization of variable ret is unnecessary, we can remove it >> >>>> while >> >>>> save >> >>>> one time "or" operation. >> >>>> >> >>>> Signed-off-by: Cong Ding <dinggnu@gmail.com> >> >>>> --- >> >>> >> >>> >> >>> Looks ok. >> >>> Acked-by: Santosh Shilimkar<santosh.shilimkar@ti.com> >> >>> >> >> >> >> 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 majordomo@vger.kernel.org >> > More majordomo info at http://vger.kernel.org/majordomo-info.html >> > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-12-24 9:00 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-12-04 11:26 [PATCH] memory: of_memory.c: remove unnecessary initialization Cong Ding 2012-12-04 11:44 ` Santosh Shilimkar 2012-12-04 13:55 ` Grant Likely 2012-12-04 14:46 ` Santosh Shilimkar 2012-12-05 7:26 ` Li, Zhen-Hua 2012-12-05 7:41 ` Li, Zhen-Hua 2012-12-05 10:06 ` Cong Ding 2012-12-24 9:00 ` Li, Zhen-Hua
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).