[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: BALATON Zoltan
Subject: Re: [PATCH] bitops.h: Compile out asserts without --enable-debug
Date: Tue, 6 Jun 2023 00:06:22 +0200 (CEST)

On Tue, 23 May 2023, Alex Bennée wrote:
Richard Henderson <richard.henderson@linaro.org> writes:
On 5/22/23 15:26, BALATON Zoltan wrote:
On Mon, 22 May 2023, Alex Bennée wrote:
(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:


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.
They aren't in code generation but in helpers as you can also see in
the profile below and so they can be on hot path. Also I've noticed
that extract8 and extract16 just call extract32 after adding another
assert on their own in addition to the one in extract32 which is
double overhead for really no reason. I'd delete all these asserts
as the likelhood of bugs these could catch is very low anyway (how
often do you expect somebody to call these with out of bound values
that would not be obvious from the results otherwise?) but leaving
them in non-debug builds is totally useless in my opinion.

So this seems to be the simplest and most effective

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               [.] 
  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
I've run 32 bit PPC version in qemu-system-ppc so the profile is a
bit different (has more system related overhead that I plan to look
at separately) but this part is similar to the above. I also wonder
what makes helper_compute_fprf_float64 a bottleneck as that does not
seem to have much extract/deposit, only a call to clz but it's hard
to tell what it really does due to nested calls and macros. I've
also seen this function among the top contenders in my profiling.

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.
Could it be related to that while the parts size and start are
marked const but pulled out of a struct field so the compiler may
not know their actual value until run time?


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

Try this.

That seems to have done the trick, translated code is now dominating the

+   14.12%     0.00%  qemu-ppc64  [unknown]                [.] 
+   13.30%     0.00%  qemu-ppc64  [unknown]                [.] 
+   12.58%    12.19%  qemu-ppc64  qemu-ppc64               [.] 
+   10.62%     0.00%  qemu-ppc64  [unknown]                [.] 
+    9.91%     9.73%  qemu-ppc64  qemu-ppc64               [.] 
+    7.84%     7.82%  qemu-ppc64  qemu-ppc64               [.] 
+    6.47%     5.78%  qemu-ppc64  qemu-ppc64               [.] 
+    6.46%     0.00%  qemu-ppc64  [unknown]                [.] 
+    6.42%     0.00%  qemu-ppc64  [unknown]                [.] 
+    6.17%     6.04%  qemu-ppc64  qemu-ppc64               [.] parts64_muladd
+    5.85%     0.00%  qemu-ppc64  [unknown]                [.] 
+    5.74%     0.00%  qemu-ppc64  [unknown]                [.] 
+    5.45%     4.78%  qemu-ppc64  qemu-ppc64               [.] 
+    4.79%     0.00%  qemu-ppc64  [unknown]                [.] 
+    4.29%     3.82%  qemu-ppc64  qemu-ppc64               [.] float64r32_muladd
+    4.20%     3.87%  qemu-ppc64  qemu-ppc64               [.] helper_todouble
+    3.51%     2.98%  qemu-ppc64  qemu-ppc64               [.] float64r32_mul
+    3.12%     2.97%  qemu-ppc64  qemu-ppc64               [.] 
+    3.06%     2.95%  qemu-ppc64  qemu-ppc64               [.] float64_add
+    2.88%     2.35%  qemu-ppc64  qemu-ppc64               [.] float64r32_add
+    2.65%     0.00%  qemu-ppc64  [unknown]                [.] 
+    2.55%     1.30%  qemu-ppc64  qemu-ppc64               [.] 

eyeballing the float functions and I can't see any asserts expressed.


Executed in  721.65 secs    fish           external
  usr time  721.38 secs  561.00 micros  721.38 secs
  sys time    0.20 secs  261.00 micros    0.20 secs


Executed in  650.38 secs    fish           external
  usr time  650.11 secs    0.00 micros  650.11 secs
  sys time    0.20 secs  989.00 micros    0.20 secs


[2. text/x-patch; z.patch]...

Will this be submitted as a proper patch and merged? This improves softfloat important for target/ppc where hardfloat is disabled. I guess you can also add

Tested-by: BALATON Zoltan <balaton@eik.bme.hu>

and Alex also did testing above. This is just pinging it so it's not forgotten. Thank you.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]