Alfie Richards <alfie.richa...@arm.com> writes: > On 18/02/2025 12:11, Richard Sandiford wrote: >> Alfie Richards <alfie.richa...@arm.com> writes: >>> This changes function version structures to maintain the default version >>> as the first declaration in the linked data structures by giving priority >>> to the set containing the default when constructing the structure. >>> >>> This allows for removing logic for moving the default to the first >>> position which was duplicated across target specific code and enables >>> easier reasoning about function sets when checking for a default. >>> >>> gcc/ChangeLog: >>> >>> * cgraph.cc (cgraph_node::record_function_versions): Update to >>> implicitly keep default first. >>> * config/aarch64/aarch64.cc (aarch64_get_function_versions_dispatcher): >>> Remove reordering. >>> * config/i386/i386-features.cc (ix86_get_function_versions_dispatcher): >>> Remove reordering. >>> * config/riscv/riscv.cc (riscv_get_function_versions_dispatcher): >>> Remove reordering. >>> * config/rs6000/rs6000.cc (rs6000_get_function_versions_dispatcher): >>> Remove reordering. >>> --- >>> gcc/cgraph.cc | 27 ++++++++++++++++----- >>> gcc/config/aarch64/aarch64.cc | 37 +++++++--------------------- >>> gcc/config/i386/i386-features.cc | 33 ++++--------------------- >>> gcc/config/riscv/riscv.cc | 41 +++++++------------------------- >>> gcc/config/rs6000/rs6000.cc | 35 +++++---------------------- >>> 5 files changed, 49 insertions(+), 124 deletions(-) >>> >>> diff --git a/gcc/cgraph.cc b/gcc/cgraph.cc >>> index d0b19ad850e..bf6b43d00db 100644 >>> --- a/gcc/cgraph.cc >>> +++ b/gcc/cgraph.cc >>> @@ -247,7 +247,9 @@ cgraph_node::record_function_versions (tree decl1, tree >>> decl2) >>> decl1_v = decl1_node->function_version (); >>> decl2_v = decl2_node->function_version (); >>> >>> - if (decl1_v != NULL && decl2_v != NULL) >>> + /* If the nodes are already linked, skip. */ >>> + if ((decl1_v != NULL && (decl1_v->next || decl1_v->prev)) >>> + && (decl2_v != NULL && (decl2_v->next || decl2_v->prev))) >>> return; >>> >>> if (decl1_v == NULL) >>> @@ -256,18 +258,31 @@ cgraph_node::record_function_versions (tree decl1, >>> tree decl2) >>> if (decl2_v == NULL) >>> decl2_v = decl2_node->insert_new_function_version (); >>> >>> - /* Chain decl2_v and decl1_v. All semantically identical versions >>> - will be chained together. */ >>> + gcc_assert (decl1_v); >>> + gcc_assert (decl2_v); >>> >>> before = decl1_v; >>> after = decl2_v; >>> >>> + /* Go to first after node. */ >>> + while (after->prev != NULL) >>> + after = after->prev; >>> + >>> + while (before->prev != NULL) >>> + before = before->prev; >>> + >>> + /* Potentially swap the nodes to maintain the default always being in the >>> + first position. */ >>> + if (before->next >>> + ? !is_function_default_version (before->this_node->decl) >>> + : is_function_default_version (after->this_node->decl)) >>> + std::swap (before, after); >>> + >>> + /* Go to last node of before. */ >>> while (before->next != NULL) >>> before = before->next; >>> >>> - while (after->prev != NULL) >>> - after= after->prev; >>> - >>> + /* Chain decl2_v and decl1_v. */ >> I think this can be simplified to: >> >> before = decl1_v; >> after = decl2_v; >> >> /* Potentially swap the nodes to maintain the default always being in the >> first position. */ >> if (before->prev || before->next >> ? is_function_default_version (after->this_node->decl) >> : !is_function_default_version (before->this_node->decl)) >> std::swap (before, after); >> >> while (before->next != NULL) >> before = before->next; >> >> while (after->prev != NULL) >> after = after->prev; >> >> That is, if one decl is linked (and so the other is not), we only want >> to put the other decl first if it is the default. > I see your point here, which I think relies on the assumption that > functions get > added to the structure one by one rather than in a fractal pattern. > This assumption is already used here subtly so that makes sense. > > I added this logic to at least try make this work in a slightly more > general case as > to tell if a structure contains the default we should check the first > element > of that structure, but it is unnecessary given that knowledge. > > I would prefer to change this to make that more explicit and change this > to be > "add_decl_to_version_into" taking a cgraph_function_version_info for the > existing structure and a decl for the version to add to make this > explicit. Would that change work for you?
Yeah, sounds good to me. I agree that it would be better than having to maintain symmetry, and it should make the interface a bit simpler. Honza should have the final say though. Thanks, Richard
