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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAEk7JKbXaT+yG6Wk_+hSOP8w=wbx8nD9NDO54aFcQ9UML2Gyqg@mail.gmail.com>
Date:	Mon, 24 Dec 2012 17:00:24 +0800
From:	"Li, Zhen-Hua" <lizhenhua.dev@...il.com>
To:	Cong Ding <dinggnu@...il.com>
Cc:	Santosh Shilimkar <santosh.shilimkar@...com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Benoit Cousson <b-cousson@...com>, Aneesh V <aneesh@...com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] memory: of_memory.c: remove unnecessary initialization

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@...il.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@...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@...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@...il.com>
>> >>>> ---
>> >>>
>> >>>
>> >>> Looks ok.
>> >>> Acked-by: Santosh Shilimkar<santosh.shilimkar@...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@...r.kernel.org
>> > 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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/