On Fri, May 30, 2025 at 11:30 AM Jan Hubicka <hubi...@ucw.cz> wrote: > > Hi, > > > > > > Hi, > > > > > > the attached Ada testcase compiled with -O2 -gnatn makes the compiler > > > crash in > > > vect_can_force_dr_alignment_p during SLP vectorization: > > > > > > if (decl_in_symtab_p (decl) > > > && !symtab_node::get (decl)->can_increase_alignment_p ()) > > > return false; > > > > > > because symtab_node::get (decl) returns a null node. The phenomenon > > > occurs > > > for a pair of twin symbols listed like so in .cgraph: > > > > > > Opt7_Pkg.T12b/17 (Opt7_Pkg.T12b) > > > Type: variable definition analyzed > > > Visibility: semantic_interposition external public artificial > > > Aux: @0x44d45e0 > > > References: > > > Referring: opt7_pkg__enum_name_table/13 (addr) > > > opt7_pkg__enum_name_table/13 > > > (addr) > > > Availability: not-ready > > > Varpool flags: initialized read-only const-value-known > > > > > > Opt7_Pkg.T8b/16 (Opt7_Pkg.T8b) > > > Type: variable definition analyzed > > > Visibility: semantic_interposition external public artificial > > > Aux: @0x7f9fda3fff00 > > > References: > > > Referring: opt7_pkg__enum_name_table/13 (addr) > > > opt7_pkg__enum_name_table/13 > > > (addr) > > > Availability: not-ready > > > Varpool flags: initialized read-only const-value-known > > > > > > with: > > > > > > opt7_pkg__enum_name_table/13 (Opt7_Pkg.Enum_Name_Table) > > > Type: variable definition analyzed > > > Visibility: semantic_interposition external public > > > Aux: @0x44d45e0 > > > References: Opt7_Pkg.T8b/16 (addr) Opt7_Pkg.T8b/16 (addr) > > > Opt7_Pkg.T12b/17 > > > (addr) Opt7_Pkg.T12b/17 (addr) > > > Referring: opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) > > > opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) > > > opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) > > > Availability: not-ready > > > Varpool flags: initialized read-only const-value-known > > > > > > being the crux of the matter. > > > > > > What happens is that symtab_remove_unreachable_nodes leaves the last > > > symbol in > > > kind of a limbo state: in .remove_symbols, we have: > > > > > > opt7_pkg__enum_name_table/13 (Opt7_Pkg.Enum_Name_Table) > > > Type: variable > > > Body removed by symtab_remove_unreachable_nodes > > > Visibility: externally_visible semantic_interposition external public > > > References: > > > Referring: opt7_pkg__image/2 (read) opt7_pkg__image/2 (read) > > > Availability: not_available > > > Varpool flags: initialized read-only const-value-known > > > > > > This means that the "body" (DECL_INITIAL) of the symbol has been > > > disregarded > > > during reachability analysis, causing the first two symbols to be > > > discarded: > > > > > > Reclaiming variables: Opt7_Pkg.T12b/17 Opt7_Pkg.T8b/16 > > > > > > but the DECL_INITIAL is explicitly preserved for later constant folding, > > > which > > > makes it possible to retrofit the DECLs corresponding to the first two > > > symbols > > > in the GIMPLE IR and ultimately leads vect_can_force_dr_alignment_p to > > > crash. > > > > > > > > > The decision to disregard the "body" (DECL_INITIAL) of the symbol is made > > > in > > > the first process_references present in ipa.cc: > > > > > > if (node->definition && !node->in_other_partition > > > && ((!DECL_EXTERNAL (node->decl) || node->alias) > > > || (possible_inline_candidate_p (node) > > > /* We use variable constructors during late compilation for > > > constant folding. Keep references alive so partitioning > > > knows about potential references. */ > > > || (VAR_P (node->decl) > > > && (flag_wpa > > > || flag_incremental_link > > > == INCREMENTAL_LINK_LTO) > > > && dyn_cast <varpool_node *> (node) > > > ->ctor_useable_for_folding_p ())))) > > > > > > because neither flag_wpa nor flag_incremental_link = INCREMENTAL_LINK_LTO > > > is > > > true, while the decision to ultimately preserve the DECL_INITIAL is made > > > later > > > in remove_unreachable_nodes: > > > > > > /* Keep body if it may be useful for constant folding. */ > > > if ((flag_wpa || flag_incremental_link == INCREMENTAL_LINK_LTO) > > > || ((init = ctor_for_folding (vnode->decl)) == error_mark_node)) > > > vnode->remove_initializer (); > > > else > > > DECL_INITIAL (vnode->decl) = init; > > > > > > > > > I think that the testcase shows that the "body" of > > > ctor_useable_for_folding_p > > > symbols must always be considered for reachability analysis (which could > > > make > > > the above test on ctor_for_folding useless). But implementing that > > > introduces > > > a regression for g++.dg/ipa/devirt-39.C, because the vtable is preserved > > > and > > > in turn forces the method to be preserved, hence the special case for > > > vtables. > > > > > > The test also renames the first process_references function in ipa.cc to > > > clear > > > the confusion with the second function in the same file. > > > > > > Bootstrapped/regtested on x86-64/Linux, OK for the mainline? > > > > Ah, I've run into the same issue with IPA PTA recently, unfortunately Honza > > seems unresponsive in bugzilla. IMO the patch looks OK, but let's give > > Honza > > the chance to chime in here - esp. the DECL_VIRTUAL special-casing is > > sth I'm not familiar with (wouldn't this apply to all COMDATs? but only > > considering whole-groups?) > Sorry for slow reaction - I am trying to catch up after end of the > semester. > > The original problem here was indeed the devirtualization. If C++ class > is keyed we will produce virtual table as extern const variable. This > table is useful for devirtualizatoin but at some time we need to be sure > to not output all function bodies (which may be COMDAT) referenced by > them (and only those which was actually used for devirtualization). > > Last time we can do it is the WPA stage, so idea is to keep all stuff > reacable by external constructor alive until inlining and remove it then > (devirtualization after inlining is not terribly useful transformation > anyway). The code is indeed trying to keep constructors around for > possible later folding whenever this is still doable. > > This does not introduce completely new problem. Some decls are not > accessible from very begining of middle-end compilation. C++ is bit > shady about what cen be used especially if -fdefault-visibility=hidden > is used which may hide the symbols referneced by virtual table in DSO. > So there is can_refer_decl_in_current_unit_p which tries to make C++ > pakcages happy and also should cover the logic on when we can still > access after the ctor has been oficicially moreved from vtable. > > So if you make everything referenced by external constructors reachable > except for vtables I think we are moving the problem of outputting dead > code to non-vtable case. > > I will need to check - does the retrofitting of decl into code go via > can_refer_decl_in_current_unit_p? In most cases this actually tests that > symbol table is present...
In the IPA PTA case I was getting decls refered to in the IL that do not have a varpool node. They "re-appeared" via constant folding from CTORs where refered to vars had been pruned from the symtab. See PR120146. So either we have to resurrect the varpool nodes once constant folding from CTORs adds explicit references or we have to forgo with the idea that symtab nodes are present (and thus we can actually check sth. like ->all_refs_explicit_p). Richard. > Honza > > > > Thanks, > > Richard. > > > > > > > > > > > 2025-05-29 Eric Botcazou <ebotca...@adacore.com> > > > > > > * ipa.cc (process_references): Rename into... > > > (mark_references): ...this. Always mark referenced external > > > variables as reachable if they are usable for folding, except > > > for vtables. > > > (symbol_table::remove_unreachable_nodes): Adjust to renaming. > > > > > > > > > 2025-05-29 Eric Botcazou <ebotca...@adacore.com> > > > > > > * gnat.dg/specs/opt7.ads: New test. > > > * gnat.dg/specs/opt7_pkg.ads: New helper. > > > * gnat.dg/specs/opt7_pkg.adb: Likewise. > > > > > > -- > > > Eric Botcazou
