[PATCH v2 06/16] Change function versions to be implicitly ordered.

[Alfie Richards]

On 18/02/2025 12:11, Richard Sandiford wrote:
> Alfie Richards <alfie.richards@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?
>> [...]
>> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
>> index 9bf7713139f..e5aa99a4965 100644
>> --- a/gcc/config/riscv/riscv.cc
>> +++ b/gcc/config/riscv/riscv.cc
>> @@ -13726,7 +13726,6 @@ riscv_get_function_versions_dispatcher (void *decl)
>>     struct cgraph_node *node = NULL;
>>     struct cgraph_node *default_node = NULL;
>>     struct cgraph_function_version_info *node_v = NULL;
>> -  struct cgraph_function_version_info *first_v = NULL;
>>   
>>     tree dispatch_decl = NULL;
>>   
>> @@ -13743,41 +13742,19 @@ riscv_get_function_versions_dispatcher (void *decl)
>>     if (node_v->dispatcher_resolver != NULL)
>>       return node_v->dispatcher_resolver;
>>   
>> -  /* Find the default version and make it the first node.  */
>> -  first_v = node_v;
>> -  /* Go to the beginning of the chain.  */
>> -  while (first_v->prev != NULL)
>> -    first_v = first_v->prev;
>> -  default_version_info = first_v;
>> -
>> -  while (default_version_info != NULL)
>> -    {
>> -      struct riscv_feature_bits res;
>> -      int priority; /* Unused.  */
>> -      parse_features_for_version (default_version_info->this_node->decl,
>> -				  res, priority);
>> -      if (res.length == 0)
>> -	break;
>> -      default_version_info = default_version_info->next;
>> -    }
>> +  /* The default node is always the beginning of the chain.  */
>> +  default_version_info = node_v;
>> +  while (default_version_info->prev)
>> +    default_version_info = default_version_info->prev;
>> +  default_node = default_version_info->this_node;
>>   
>>     /* If there is no default node, just return NULL.  */
>> -  if (default_version_info == NULL)
>> +  struct riscv_feature_bits res;
>> +  int priority; /* Unused.  */
>> +  parse_features_for_version (default_node->decl, res, priority);
>> +  if (res.length != 0)
>>       return NULL;
> My comment from the previous review still stands:
>
>    I expect you were trying to avoid editorialising, but: how about
>    converting this to is_function_default_version at the same time?
>    That seems more logically consistent with the target-independent code
>    and wasn't an option open to the RISC-V code until patch 5.
>
>    To put it another way, not being able to use is_function_default_version
>    here would suggest that the "default-first" assumption doesn't in fact
>    hold for RISC-V.
>
> I.e. I think this can just be:
>
>    if (!is_function_default_version (default_node->decl))
>      return NULL;
>
> as for the others.
Apologies for this, I meant to change this and must have missed it.
I agree with your reasoning.
> OK for GCC 16 with those changes, if you agree with them.
>
> Thanks,
> Richard