Re: Want comments regarding patch
> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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'
--
Re: Want comments regarding patch
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'
--
Re: Want comments regarding patch
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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
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
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.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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 <[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 InformaticaFono: +56 32 2654431 Universidad Tecnica Federico Santa Maria +56 32 2654239 Casilla 110-V, Valparaiso, Chile Fax: +56 32 2797513 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Want comments regarding patch
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Want comments regarding patch
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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 InformaticaFono: +56 32 2654431 Universidad Tecnica Federico Santa Maria +56 32 2654239 Casilla 110-V, Valparaiso, Chile Fax: +56 32 2797513 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Want comments regarding patch
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'
--
