> 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).
The actual problem is that we never really promised that all decls will have symbol nodes in late compilation since late optimizers intorduce new symbols without caring about keeping callgraph up-to-date. One example is i.e. when we introduce new explicit builtin. So the existence of symbol nodes was optional from the start. If one wants to look up something in symbol table during late optimization one is supposed to check that ::get does not return NULL and be conservative otheriwse. So your change for PR120156 was OK with me, but I did not explicitly write (sorty for that). Similarly for vectorizer I would simply test for node to exist. I suppose it is time to change that (since real symbol table should have all symbols, but that is not really backportable). I will look into that external ctor handling. It is a hack which also does not play well with partitioning. I.e. should we partition only to enable possible devirtualzation in ohter unit? So I think correct method would be to keep constructors initialized but understand in unreachable code removal that such references does not keep targets alive. Comdats are kind of tricky, since once we decide they are unreachable (except for external ctors) we should turn them into nodes that can not be referenced, but still can be constant folded through. In C++ this happens early, too, since external definition may refer to symbols static in other translation unit. I think only important case of late devirtualization is when we earlier devirtualized speculatively and later we confirm the speculation. This is actually quite common and I am thiking of using this mechanism also for normal indirect calls using parameters passed by reference (i.e. std::function). I.e. keep speculative passtru functions which was not fully disambiguated in the alias walk, but will hopefully be disambiguated at late optimization. Honza
