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
[email protected] as well? That way others can chime in as
well.
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
Daniel Marjamäki <[email protected]> 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
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'
--
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
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'
--
Jan Engelhardt <[email protected]> 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.
On Dec 30 2006 01:00, Bodo Eggert wrote:
>Jan Engelhardt <[email protected]> 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'
--
> 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