Hi!
Thanks for posting this again. Much easier to find that way :-)
On Mon, Jun 17, 2024 at 07:15:48PM +0200, Jakub Jelinek wrote:
> While my r15-1001-g4cf2de9b5268224 PCH PIE power fix change decreased the
> .data section sizes (219792 -> 189336), it increased the size of already
> huge rs6000_init_generated_builtins generated function, from 218328
> to 228668 bytes. That is because there are thousands of array references
> to global arrays and we keep constructing the addresses of the arrays
> again and again.
Less than 5%, for some perspective ;-)
> Ideally some optimization would figure out we have a single function which
> has
> 461 rs6000_overload_info
> 1257 rs6000_builtin_info_fntype
> 1768 rs6000_builtin_decls
> 2548 rs6000_instance_info_fntype
> array references and that maybe it might be a good idea to just preload
> the addresses of those arrays into some register if it decreases code size
> and doesn't slow things down.
> The function actually is called just once and is huge, so code size is even
> more important than speed, which is dominated by all the GC allocations
> anyway.
Yup.
> Until that is done, here is a slightly cleaner version of the hack, which
> makes the function noipa (so that LTO doesn't undo it) for GCC 8.1+ and
> passes the 4 arrays as arguments to the function from the caller.
> This decreases the function size from 228668 bytes to 207572 bytes.
>
> Bootstrapped/regtested on powerpc64le-linux, ok for trunk?
> 2024-06-17 Jakub Jelinek <ja...@redhat.com>
>
> PR target/115324
> * config/rs6000/rs6000-gen-builtins.cc (write_decls): Change
> declaration of rs6000_init_generated_builtins from no arguments
> to 4 pointer arguments.
> (write_init_bif_table): Change rs6000_builtin_info_fntype to
> builtin_info_fntype and rs6000_builtin_decls to builtin_decls.
> (write_init_ovld_table): Change rs6000_instance_info_fntype to
> instance_info_fntype, rs6000_builtin_decls to builtin_decls and
> rs6000_overload_info to overload_info.
> (write_init_file): Add __noipa__ attribute to
> rs6000_init_generated_builtins for GCC 8.1+ and change the function
> from no arguments to 4 pointer arguments. Change rs6000_builtin_decls
> to builtin_decls.
> * config/rs6000/rs6000-builtin.cc (rs6000_init_builtins): Adjust
> rs6000_init_generated_builtins caller.
It would have been much easier to review if you had done the renaming in
a separate patch :-) You typically notice such things when writing the
changelog is much harder than expected, and this is the True Value of
changelogs!
Seen from the other side, when reviewing a patch I like to start with
the changelog (after the commit message), it should tell everything
there is to know, and then if something in the actiual patch surprises
me, something is not ideal, or wrong even.
> + /* The reason to pass pointers to the function instead of accessing
> + the rs6000_{{builtin,instance}_info_fntype,overload_info,builtin_decls}
> + arrays directly is to decrease size of the already large function and
> + noipa prevents the compiler with LTO to undo that optimization. */
Some of these array names no longer have the rs6000_ prefix now. Oh
wait, you already took that into account? I'm not saying anything :-)
The patch is fine for trunk, thank you! If you want backports those
are okay, too (but I don't think you want any? Or does this work
withput the previous patches as well?)
Segher
