From: David Guillen Fandos <david@davidgf.es>
To: musl@lists.openwall.com
Subject: Re: Do not use 64 bit division if possible
Date: Sun, 26 Nov 2017 01:49:09 +0100 [thread overview]
Message-ID: <796e366e-f321-25a3-78e7-8a3800e62eeb@davidgf.es> (raw)
In-Reply-To: <A3F1F946-D13B-4070-B8AD-3786B54C83B4@mac.com>
Hey,
Wow that's an awesome optimization (the a&-a), didn't know gcc was smart
enough to figure that out by itself :D
I just realized that PAGE_SIZE seems indeed to be defined to a constant
for some architectures, did not notice since I was running on MIPS which
has a page size different for each uarch.
I'd say the (a&-a) is a very simple optimization and we should use it,
since it adds almost no complexity and sames some cycles and some .text
bytes, which is sometimes a bit tight.
Something like this? Doesn't hurt constants, improves some arches :)
diff --git a/src/conf/sysconf.c b/src/conf/sysconf.c
index b8b761d0..aa9fc9d1 100644
--- a/src/conf/sysconf.c
+++ b/src/conf/sysconf.c
@@ -206,7 +206,7 @@ long sysconf(int name)
if (name==_SC_PHYS_PAGES) mem = si.totalram;
else mem = si.freeram + si.bufferram;
mem *= si.mem_unit;
- mem /= PAGE_SIZE;
+ mem /= (unsigned)(PAGE_SIZE & -PAGE_SIZE);
return (mem > LONG_MAX) ? LONG_MAX : mem;
case JT_ZERO & 255:
return 0;
On 26/11/17 01:10, Michael Clark wrote:
>
>
>> On 26/11/2017, at 12:53 PM, Rich Felker <dalias@libc.org> wrote:
>>
>> On Sun, Nov 26, 2017 at 12:46:56AM +0100, David Guillen Fandos wrote:
>>> Thanks for your response.
>>> Please note that PAGE_SIZE is not a constant but an alias to
>>> libc.page_size which is a variable of type size_t (signed).
>>> That's why at O1+ gcc doesn't generate a shift.
>>
>> Indeed; this varies by arch.
>
> Oh, I wasn’t aware of that.
>
>>> I also created a patch to include libc.page_shift, but as far as I
>>> can see no other functions would benefit from it, since there's no
>>> other divides there (only negations, additions and subtractions).
>>
>> Adding infrastructure complexity except in cases where it makes a
>> significant improvement to size or performance is generally not
>> desirable. mmap() is one other place where, in principle, division by
>> PAGE_SIZE might take place, but in practice the size is constant 4096
>> or 8192 on all archs.
>>
>>> And yeah I agree, a_ctz_l is not exactly inexpensive but I guess it
>>> is better than full 64 bit signed division (that's why I cast
>>> unsigned otherwise the shift right is not trivial due to the sign).
>>
>> The cost here is more a matter of adding a reading complexity
>> dependency on musl internals (a_*) where it's not needed. I wonder if
>> GCC could optimize it if we instead of /PAGE_SIZE wrote
>> /(PAGE_SIZE&-PAGE_SIZE). Or if we did something like define PAGE_SIZE
>> as ((libc.page_size&-libc.page_size)==libc.page_size ? libc.page_size
>> : 1/0) so that "PAGE_SIZE is not a power of 2" would become an
>> unreachable case.
>
> Interesting. It seems GCC figures out the division by zero is unreachable but the (n&-n) expression leads to a power of two, not to a log2 n so the ctz is still required.
>
> - https://cx.rv8.io/g/eHf2Ah
>
> One could do so once at initialisation time and add PAGE_SHIFT and on architectures with variable page sizes do this:
>
> #define PAGE_SHIFT libc.page_shift
>
> diff --git a/src/env/__libc_start_main.c b/src/env/__libc_start_main.c
> index 2d758af..f24d10a 100644
> --- a/src/env/__libc_start_main.c
> +++ b/src/env/__libc_start_main.c
> @@ -29,6 +29,7 @@ void __init_libc(char **envp, char *pn)
> __hwcap = aux[AT_HWCAP];
> __sysinfo = aux[AT_SYSINFO];
> libc.page_size = aux[AT_PAGESZ];
> + libc.page_shift = a_ctz_l(libc.page_size);
>
> if (!pn) pn = (void*)aux[AT_EXECFN];
> if (!pn) pn = "";
>
> That isolates the a_ctz_l to one place.
>
next prev parent reply other threads:[~2017-11-26 0:49 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-25 20:52 David Guillen Fandos
2017-11-25 23:15 ` Michael Clark
2017-11-25 23:46 ` David Guillen Fandos
2017-11-25 23:53 ` Rich Felker
2017-11-26 0:10 ` Michael Clark
2017-11-26 0:49 ` David Guillen Fandos [this message]
2017-11-26 0:59 ` Rich Felker
2017-11-26 1:12 ` David Guillen Fandos
2017-11-26 1:23 ` Rich Felker
2017-11-26 1:40 ` David Guillen Fandos
2017-11-26 0:49 ` Rich Felker
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=796e366e-f321-25a3-78e7-8a3800e62eeb@davidgf.es \
--to=david@davidgf.es \
--cc=musl@lists.openwall.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).