(ajb: add Richard for his compiler-fu)
BALATON Zoltan <balaton@eik.bme.hu> writes:
On Mon, 22 May 2023, Alex Bennée wrote:
BALATON Zoltan <balaton@eik.bme.hu> writes:
The low level extract and deposit funtions provided by bitops.h are
used in performance critical places. It crept into target/ppc via
FIELD_EX64 and also used by softfloat so PPC code using a lot of FPU
where hardfloat is also disabled is doubly affected.
Most of these asserts compile out to nothing if the compiler is able to
verify the constants are in the range. For example examining
the start of float64_add:
<snip>
I don't see any check and abort steps because all the shift and mask
values are known at compile time. The softfloat compilation certainly
does have some assert points though:
readelf -s ./libqemu-ppc64-softmmu.fa.p/fpu_softfloat.c.o |grep assert
136: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND g_assertion_mess[...]
138: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UND __assert_fail
but the references are for the ISRA segments so its tricky to know if
they get used or are just there for LTO purposes.
If there are hot-paths that show up the extract/deposit functions I
suspect a better approach would be to implement _nocheck variants (or
maybe _noassert?) and use them where required rather than turning off
the assert checking for these utility functions.
Just to clarify again, the asserts are still there when compiled with
--enable-debug. The patch only turns them off for optimised release
builds which I think makes sense if these asserts are to catch
programming errors.
Well as Peter said the general policy is to keep asserts in but I
appreciate this is a hotpath case.
I think I've also suggested adding noassert
versions of these but that wasn't a popular idea and it may also not
be easy to convert all places to use that like for example the
register fields related usage in target/ppc as that would also affect
other places.
Is code generation or device emulation really on the hot-path. Generally
a well predicted assert is in the noise for those operations.
So this seems to be the simplest and most effective
approach.
The softfloat related usage in these tests I've done seem to mostly
come from unpacking and repacking floats in softfloat which is done
for every operation, e.g. muladd which mp3 encoding mostly uses does 3
unpacks and 1 pack for each call and each unpack is 3 extracts so even
small overheads add app quickly. Just 1 muladd will result in 9
extracts and 2 deposits at least plus updating PPC flags for each FPU
op adds a bunch more. I did some profiling with perf to find these.
After some messing about trying to get lame to cross compile to a static
binary I was able to replicate what you've seen:
11.44% qemu-ppc64 qemu-ppc64 [.] unpack_raw64.isra.0
11.03% qemu-ppc64 qemu-ppc64 [.] parts64_uncanon_normal
8.26% qemu-ppc64 qemu-ppc64 [.] helper_compute_fprf_float64
6.75% qemu-ppc64 qemu-ppc64 [.] do_float_check_status
5.34% qemu-ppc64 qemu-ppc64 [.] parts64_muladd
4.75% qemu-ppc64 qemu-ppc64 [.] pack_raw64.isra.0
4.38% qemu-ppc64 qemu-ppc64 [.] parts64_canonicalize
3.62% qemu-ppc64 qemu-ppc64 [.]
float64r32_round_pack_canonical
3.32% qemu-ppc64 qemu-ppc64 [.] helper_todouble
2.68% qemu-ppc64 qemu-ppc64 [.] float64_add
2.51% qemu-ppc64 qemu-ppc64 [.] float64_hs_compare
2.30% qemu-ppc64 qemu-ppc64 [.] float64r32_muladd
1.80% qemu-ppc64 qemu-ppc64 [.] float64r32_mul
1.40% qemu-ppc64 qemu-ppc64 [.] float64r32_add
1.34% qemu-ppc64 qemu-ppc64 [.] parts64_mul
1.16% qemu-ppc64 qemu-ppc64 [.] parts64_addsub
1.14% qemu-ppc64 qemu-ppc64 [.] helper_reset_fpstatus
1.06% qemu-ppc64 qemu-ppc64 [.] helper_float_check_status
1.04% qemu-ppc64 qemu-ppc64 [.] float64_muladd
z.patch