Want comments regarding patch

Want comments regarding patch

[Daniel Marjamäki]

Hello all!

I sent a patch with this content:

-       for (i = 0; i < MAX_PIRQS; i++)
-               pirq_entries[i] = -1;
+       memset(pirq_entries, -1, sizeof(pirq_entries));

I'd like to know if you have any comments to this change. It was of
course my intention to make the code shorter, simpler and faster.

I've discussed this with Ingo Molnar and here's our conversation:

INGO:

hm, i'm not sure i like this - the '-1' in the memset is for a byte,
while the pirq_entries are word sized. It should work because the bytes
happen to be 0xff for the word too, but this is encodes an assumption,
and were we ever to change that value it could break silently. gcc ought
to be able to figure out the best way to initialize the array.


DANIEL:

Thank you for the comments.

I understand your point, it's good. But I personally still like my
method better.
For me -1 is just as valid as an argument as 0. As you note however,
it assumes that the next developer understands the encoding of
negative numbers. A developer who doesn't know the encoding could be
very confused. Would my patch be ok if I used '0xff' instead of '-1'?

With version 3.3.6 (gcc) there's quite a big difference in the
assembly code (between 'for' and 'memset').

INGO:

0xff might be better, but i'm still uneasy about it ... No other piece
of code within the kernel does this.

could you post the patch and the reasoning to
linux-kernel@vger.kernel.org as well? That way others can chime in as
well.

Re: Want comments regarding patch

[Arjan van de Ven]

On Thu, 2006-12-28 at 19:41 +0100, Daniel Marjamäki wrote:
> Hello all!
> 
> I sent a patch with this content:
> 
> -       for (i = 0; i < MAX_PIRQS; i++)
> -               pirq_entries[i] = -1;
> +       memset(pirq_entries, -1, sizeof(pirq_entries));
> 
> I'd like to know if you have any comments to this change. It was of
> course my intention to make the code shorter, simpler and faster.

Hi,

personally I don't like the new code; memset only takes a byte as
argument and while it probably is the same, that is now implicit
behavior and no longer explicit. A reasonably good compiler will notice
it's the same and generate the best code anyway, so I would really
really suggest to go for the best readable code, which imo is the
original code.

Greetings,
   Arjan van de Ven

Re: Want comments regarding patch

[Jan Engelhardt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1383 bytes --]


On Dec 28 2006 19:53, Arjan van de Ven wrote:
>On Thu, 2006-12-28 at 19:41 +0100, Daniel Marjamäki wrote:
>> Hello all!
>> 
>> I sent a patch with this content:
>> 
>> -       for (i = 0; i < MAX_PIRQS; i++)
>> -               pirq_entries[i] = -1;
>> +       memset(pirq_entries, -1, sizeof(pirq_entries));
>> 
>> I'd like to know if you have any comments to this change. It was
>> of course my intention to make the code shorter, simpler and
>> faster.
>
>personally I don't like the new code; memset only takes a byte as
>argument and while it probably is the same, that is now implicit
>behavior and no longer explicit. A reasonably good compiler will
>notice it's the same and generate the best code anyway, so I would
>really really suggest to go for the best readable code, which imo is
>the original code.

Then GCC is not a "reasonably good compiler". Considering

#define MAX 6400
struct foo {
    int line[MAX];
};
void bar(struct foo *a) {
    int i;
    for(i = 0; i < MAX; ++i)
        a->line[i] = -1;
}
void baz(struct foo *a) {
    __builtin_memset(a->line, -1, sizeof(a->line));
}

`gcc -O3 -c test.c` will generate a classic loop rather than a repz
movsd for bar(). baz() will get a call to an extern memset(),
probably because gcc could not figure out how to make a repz for it
and hence thought it was better to use an external hand-crafted
memset.


	-`J'
-- 

Re: Want comments regarding patch

[Daniel Marjamäki]

Hello!

Thank you for your comments. It seems to me the issue was the readability.

It was my goal to improve the readability. I failed.

I personally prefer to use standard functions instead of writing code.
In my opinion using standard functions means less code that is easier to read.

Best regards,
Daniel

Re: Want comments regarding patch

[Jan Engelhardt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 579 bytes --]


On Dec 29 2006 07:57, Daniel Marjamäki wrote:
>
> It was my goal to improve the readability. I failed.
>
> I personally prefer to use standard functions instead of writing code.
> In my opinion using standard functions means less code that is easier to read.

Hm in that case, what about having something like

void *memset_int(void *a, int x, int n) {
    asm("mov %0, %%esi;
         mov %1, %%eax;
         mov %2, %%ecx;
         repz movsd;",
       a,x,n);
}

in include/asm-i386/ and asm-x86_64/? (For x86_64, also a memset_long 
that uses rsi/rax/rcx/movsq)


	-`J'
-- 

Re: Want comments regarding patch

[Horst H. von Brand]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 971 bytes --]

Daniel Marjamäki <daniel.marjamaki@gmail.com> wrote:
> I sent a patch with this content:
> 
> -       for (i = 0; i < MAX_PIRQS; i++)
> -               pirq_entries[i] = -1;
> +       memset(pirq_entries, -1, sizeof(pirq_entries));
> 
> I'd like to know if you have any comments to this change. It was of
> course my intention to make the code shorter, simpler and faster.

- Is this code in some fast path? If so, I'd set up a constant array that
  is memcpy()'d over the variable (or used to initialize it) as needed.
- If not, what is the point? Shaving off a few bytes/cycles and paying for
  that with massive developer confusion? What if the constant changes and
  is -2, or 1, tomorrow?
-- 
Dr. Horst H. von Brand                   User #22616 counter.li.org
Departamento de Informatica                    Fono: +56 32 2654431
Universidad Tecnica Federico Santa Maria             +56 32 2654239
Casilla 110-V, Valparaiso, Chile               Fax:  +56 32 2797513

Re: Want comments regarding patch

[Bodo Eggert]

Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:
> On Dec 29 2006 07:57, Daniel Marjamäki wrote:

>> It was my goal to improve the readability. I failed.
>>
>> I personally prefer to use standard functions instead of writing code.
>> In my opinion using standard functions means less code that is easier to
>> read.
> 
> Hm in that case, what about having something like
> 
> void *memset_int(void *a, int x, int n) {
>     asm("mov %0, %%esi;
>          mov %1, %%eax;
>          mov %2, %%ecx;
>          repz movsd;",
>        a,x,n);
> }

This would copy the to-be-initialized buffer somewhere, if it compiles.

1) You want stosd, "store string", not "move string"
2) You'll want to set %%di (destination index) instead of %%si.
3) repz should be illegal for movs, it might be interpreted as rep by
   defective assemblers, since it generates the same prefix. "rep" is
   correct here, since you don't want to break on (non-)zero-words.
4) Mind the direction flag.

Re: Want comments regarding patch

[Jan Engelhardt]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 890 bytes --]


On Dec 30 2006 01:00, Bodo Eggert wrote:
>Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:
>> On Dec 29 2006 07:57, Daniel Marjamäki wrote:
>
>>> It was my goal to improve the readability. I failed.
>>>
>>> I personally prefer to use standard functions instead of writing code.
>>> In my opinion using standard functions means less code that is easier to
>>> read.
>> 
>> Hm in that case, what about having something like
>> 
>> void *memset_int(void *a, int x, int n) {
>>     asm("mov %0, %%esi;
>>          mov %1, %%eax;
>>          mov %2, %%ecx;
>>          repz movsd;",
>>        a,x,n);
>> }
>
>This would copy the to-be-initialized buffer somewhere, if it compiles.

Yeah I don't do assembler soo often that I would know everything from heart.
All your comments are valid of course. I just wanted to point out the idea.
(However, if it's not repz, then it's repnz! :-)

	-`J'
-- 

Re: Want comments regarding patch

[Arjan van de Ven]

> Yeah I don't do assembler soo often that I would know everything from heart.
> All your comments are valid of course. I just wanted to point out the idea.
> (However, if it's not repz, then it's repnz! :-)

it's better to use a gcc builtin than handcoding the asm yourself;
better for performance at least....
(gcc will pick the best code for the cpu you picked, and yes this
changes every generation or so)

-- 
if you want to mail me at work (you don't), use arjan (at) linux.intel.com
Test the interaction between Linux and your BIOS via http://www.linuxfirmwarekit.org