Date: Mon, 16 May 2011 00:52:13 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Marius Strobl <marius@alchemy.franken.de> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, Bruce Evans <brde@optusnet.com.au> Subject: Re: svn commit: r221913 - head/sys/dev/mii Message-ID: <20110516002156.T911@besplex.bde.org> In-Reply-To: <20110515105004.GG92688@alchemy.franken.de> References: <201105142031.p4EKV5el014531@svn.freebsd.org> <20110515135737.B825@besplex.bde.org> <20110515105004.GG92688@alchemy.franken.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, 15 May 2011, Marius Strobl wrote:
> On Sun, May 15, 2011 at 02:35:37PM +1000, Bruce Evans wrote:
>> On Sat, 14 May 2011, Marius Strobl wrote:
>>
>>> Log:
>>> - There's no need for nibbletab to be static, it's const however.
>>> - Fix whitespace.
>>
>> There is every reason for it to be static.
>>
>>> Modified: head/sys/dev/mii/mii.c
>>> ==============================================================================
>>> --- head/sys/dev/mii/mii.c Sat May 14 19:36:12 2011 (r221912)
>>> +++ head/sys/dev/mii/mii.c Sat May 14 20:31:04 2011 (r221913)
>>> @@ -552,7 +552,7 @@ mii_down(struct mii_data *mii)
>>> static unsigned char
>>> mii_bitreverse(unsigned char x)
>>> {
>>> - static unsigned char nibbletab[16] = {
>>> + unsigned const char const nibbletab[16] = {
>>> 0, 8, 4, 12, 2, 10, 6, 14, 1, 9, 5, 13, 3, 11, 7, 15
>>> };
>>
>> Making it non-static asks the compiler to pessimize it by initializing
>> it on every entry to the function. Some versions of gcc do a very good
>
> This function is mostly used during device probing (and the result of
> the caller cached afterwards) and not in the hot path, thus there's no
> need to tell the compiler that it _must_ keep a static copy of the array.
Actually, in a more realistic example, and in this function, it is impossible
for the compiler to avoid static storage. The result probably depends on
the arg, in a form like table[arg] Better example below.
Another good pessimization in the above is passing the arg as an unsigned
char. The caller may have to demote to this type, and the function will
probably have to promote it to use it as an index. Similarly for returning
a type shorter than int. These pessimizations are also shown in the
example below.
> ...
> I agree that such code shoudn't be used in the hot path.
I agree that this doesn't matter much for efficiency.
Auto initializers are also worse for debugging (without full debugging info
or debuggers that understand it).
Better example:
%%%
$ cat z.c
unsigned char data;
int result;
unsigned char
foo(unsigned char n)
{
const unsigned char foo[16] = { 1, 2, 3, 4, };
return (foo[n]);
}
void
use(void)
{
result += foo(data);
}
$ gcc42 -O2 -S z.c
$ cat z.s [edited]
% .file "z.c"
% .text
% .p2align 4,,15
% .globl foo
% .type foo, @function
% foo:
% pushl %ebp
% movl %esp, %ebp
% movzbl 8(%ebp), %eax
Extra work to promote the arg. movzbl instead of movl normally only costs
space on modern x86.
% popl %ebp
% movzbl foo.1543(%eax), %eax
The table needs to be of unsigned char's to optimize for space. Although
we only return an unsigned char byte, the ABI may bogusly require this movzbl.
% ret
% .size foo, .-foo
% .p2align 4,,15
% .globl use
% .type use, @function
% use:
% pushl %ebp
% movl %esp, %ebp
% subl $4, %esp
% movzbl data, %eax
% movl %eax, (%esp)
Extra work to promote the arg. Although we start with an unsigned char
and the function takes an unsigned char, the ABI may bogusly require this
movzbl. Avoiding partial register stalls and memory store-to-load mismatches
may also require this movzbl. But I think only making the ABI portable to
K&R code requires this. Storing garbage from the top bits of %eax after
movb to %al might cause a stall, but storing only %al wouldn't, and the
garbage in memory wouldn't cause any problems, since the pessimizations in
the caller result in only the low bits in memory being loaded.
% call foo
% movzbl %al, %eax
Extra work to promote the result of foo(). Although the caller has already
promoted it, the ABI might not allow depending on this.
% addl %eax, result
% leave
% ret
% .size use, .-use
% .section .rodata
% .type foo.1543, @object
% .size foo.1543, 16
% foo.1543:
% .byte 1
% .byte 2
% .byte 3
% .byte 4
% .zero 12
The compiler can't avoid static allocation for this area.
The worseness of auto initializers for debugging is also shown here.
Instead of a static array named foo, there is a static array with
mangled name foo.1543. This mangled name is not too hard to guess
or type in a primitive debugger like ddb (except in old versions of
ddb that didn't support dots in identifiers), but others might be
harder.
% .comm data,1,1
% .comm result,4,4
% .ident "GCC: (GNU) 4.2.1 20070719 [FreeBSD]"
%
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20110516002156.T911>