On Thu, May 29, 2025 at 11:38 AM Eric Botcazou <botca...@adacore.com> wrote: > > 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?) 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
