Hi!
Jakub, thanks for fixing this. I didn't realize the PCH implications here,
clearly...
On 2/1/22 12:33 PM, Segher Boessenkool wrote:
> Hi!
>
> On Tue, Feb 01, 2022 at 04:27:40PM +0100, Jakub Jelinek wrote:
>> +/* PR target/104323 */
>> +/* { dg-require-effective-target powerpc_altivec_ok } */
>> +/* { dg-options "-maltivec" } */
>> +
>> +#include <altivec.h>
>> testcase which I'm not including into testsuite because for some reason
>> the test fails on non-powerpc* targets (is done even on those and fails
>> because of missing altivec.h etc.),
> powerpc_altivec_ok returns false if the target isn't Power, you can use
> this in the testsuite fine? Why does it still fail on other targets,
> the test should be SKIPPED there?
>
> Or wait, proc check_effective_target_powerpc_altivec_ok is broken, and
> does not implement its intention or documentation. Will fix.
>
>> PCH is broken on powerpc*-*-* since the
>> new builtin generator has been introduced.
>> The generator contains or emits comments like:
>> /* #### Cannot mark this as a GC root because only pointer types can
>> be marked as GTY((user)) and be GC roots. All trees in here are
>> kept alive by other globals, so not a big deal. Alternatively,
>> we could change the enum fields to ints and cast them in and out
>> to avoid requiring a GTY((user)) designation, but that seems
>> unnecessarily gross. */
>> Having the fntypes stored in other GC roots can work fine for GC,
>> ggc_collect will then always mark them and so they won't disappear from
>> the tables, but it definitely doesn't work for PCH, which when the
>> arrays with fntype members aren't GTY marked means on PCH write we create
>> copies of those FUNCTION_TYPEs and store in *.gch that the GC roots should
>> be updated, but don't store that rs6000_builtin_info[?].fntype etc. should
>> be updated. When PCH is read again, the blob is read at some other address,
>> GC roots are updated, rs6000_builtin_info[?].fntype contains garbage
>> pointers (GC freed pointers with random data, or random unrelated types or
>> other trees).
>> The following patch fixes that. It stops any user markings because that
>> is totally unnecessary, just skips fields we don't need to mark and adds
>> GTY(()) to the 2 array variables. We can get rid of all those global
>> vars for the fn types, they can be now automatic vars.
>> With the patch we get
>> {
>> &rs6000_instance_info[0].fntype,
>> 1 * (RS6000_INST_MAX),
>> sizeof (rs6000_instance_info[0]),
>> >_ggc_mx_tree_node,
>> >_pch_nx_tree_node
>> },
>> {
>> &rs6000_builtin_info[0].fntype,
>> 1 * (RS6000_BIF_MAX),
>> sizeof (rs6000_builtin_info[0]),
>> >_ggc_mx_tree_node,
>> >_pch_nx_tree_node
>> },
>> as the new roots which is exactly what we want and significantly more
>> compact than countless
>> {
>> &uv2di_ftype_pudi_usi,
>> 1,
>> sizeof (uv2di_ftype_pudi_usi),
>> >_ggc_mx_tree_node,
>> >_pch_nx_tree_node
>> },
>> {
>> &uv2di_ftype_lg_puv2di,
>> 1,
>> sizeof (uv2di_ftype_lg_puv2di),
>> >_ggc_mx_tree_node,
>> >_pch_nx_tree_node
>> },
>> {
>> &uv2di_ftype_lg_pudi,
>> 1,
>> sizeof (uv2di_ftype_lg_pudi),
>> >_ggc_mx_tree_node,
>> >_pch_nx_tree_node
>> },
>> {
>> &uv2di_ftype_di_puv2di,
>> 1,
>> sizeof (uv2di_ftype_di_puv2di),
>> >_ggc_mx_tree_node,
>> >_pch_nx_tree_node
>> },
>> cases (822 of these instead of just those 4 shown).
> Bill, can you review the builtin side of this?
Yes, I've just read through it and it looks just fine to me.
It's a big improvement over what I had there, even ignoring
the PCH issues.
Thanks again, Jakub!
Bill
>
>> PR target/104323
>> * config/rs6000/t-rs6000 (EXTRA_GTYPE_DEPS): Append rs6000-builtins.h
>> rather than $(srcdir)/config/rs6000/rs6000-builtins.def.
>> * config/rs6000/rs6000-gen-builtins.cc (write_decls): Don't use
>> GTY((user)) for struct bifdata and struct ovlddata. Instead add
>> GTY((skip(""))) to members with pointer and enum types that don't need
>> to be tracked. Add GTY(()) to rs6000_builtin_info and
>> rs6000_instance_info
>> declarations. Don't emit gt_ggc_mx and gt_pch_nx declarations.
> Nice :-)
>
>> (write_extern_fntype, write_fntype): Remove.
>> (write_fntype_init): Emit the fntype vars as automatic vars instead
>> of file scope ones.
>> (write_header_file): Don't iterate with write_extern_fntype.
>> (write_init_file): Don't iterate with write_fntype. Don't emit
>> gt_ggc_mx and gt_pch_nx definitions.
>> if (tf_found)
>> - fprintf (init_file, " if (float128_type_node)\n ");
>> + fprintf (init_file,
>> + " tree %s = NULL_TREE;\n if (float128_type_node)\n ",
>> + buf);
>> else if (dfp_found)
>> - fprintf (init_file, " if (dfloat64_type_node)\n ");
>> + fprintf (init_file,
>> + " tree %s = NULL_TREE;\n if (dfloat64_type_node)\n ",
>> + buf);
> Things are much more readable if you break this into multiple print
> statements. I realise the existomg code is like that, but that could
> be improved.
>
> So, okay for trunk (modulo what Bill finds), and you can include the
> testcase as well after I have fixed the selector. Thanks Jakub!
>
>
> Segher
