Re: [PATCH] bitops.h: Compile out asserts without --enable-debug

["Alex Bennée" <alex.bennee@linaro.org>]

(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

what I find confusing is the fact the parts extraction and packing
should all be known constants which should cause the asserts to
disappear. However it looks like the compiler decided to bring in a copy
of the whole inline function (ISRA = >interprocedural scalar replacement
of aggregates) which obviously can't fold the constants and eliminate
the assert.

Richard,

Any idea of why the compiler might decide to do something like this?

>
> Regards,
> BALATON Zoltan


--
Alex Bennée
Virtualisation Tech Lead @ Linaro