Re: [PATCH] memory: of_memory.c: remove unnecessary initialization

[Cong Ding]

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@xxxxxx> 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@xxxxxx> 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@xxxxxxxxx>
> >>>> ---
> >>>
> >>>
> >>> Looks ok.
> >>> Acked-by: Santosh Shilimkar<santosh.shilimkar@xxxxxx>
> >>>
> >>
> >> 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@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/