The 08/01/2025 14:07, Richard Sandiford wrote:
> Alfie Richards <alfie.richa...@arm.com> writes:
> > On 01/08/2025 11:46, Richard Sandiford wrote:
> >> Sorry, I think I missed the multiple_targets.cc changes in my
> >> previous review.
> >>
> >> Alfie Richards <alfie.richa...@arm.com> writes:
> >>> +
> >>> + tree attrs = remove_attribute ("target_clones",
> >>> + DECL_ATTRIBUTES (node->decl));
> >>> + tree assembler_name = node_v->assembler_name;
> >>> +
> >>> + /* Change the current node into the default node. */
> >>> + if (num_defaults == 1)
> >>> {
> >>> - char *attr = attrs[i];
> >>> + /* Setting new attribute to initial function. */
> >>> + tree attributes = make_attribute (new_attr_name, "default", attrs);
> >>> + DECL_ATTRIBUTES (node->decl) = attributes;
> >>> + DECL_FUNCTION_VERSIONED (node->decl) = true;
> >>> +
> >>> + node->is_target_clone = true;
> >>> + node->local = false;
> >>> +
> >>> + /* Remangle base node after new target version string set. */
> >>> + tree id = targetm.mangle_decl_assembler_name (node->decl,
> >>> assembler_name);
> >>> + symtab->change_decl_assembler_name (node->decl, id);
> >>> + }
> >>> + else
> >>> + {
> >>> + /* Target clones without a default are only allowed for
> >>> target_version
> >>> + semantics where we can have target_clones/target_version mixing. */
> >>> + gcc_assert (!TARGET_HAS_FMV_TARGET_ATTRIBUTE);
> >>> +
> >>> + /* If there isn't a default version, can safely remove this
> >>> version.
> >>> + The node itself gets removed after the other versions are created. */
> >>> + cgraph_function_version_info *temp = node_v;
> >>> + node_v = node_v->next ? node_v->next : node_v->prev;
> >>> + cgraph_node::delete_function_version (temp);
> >>> + }
> >>
> >> In terms of staging, would it be worth putting the else block in patch 8
> >> instead, along with the corresponding !node_v and num_defaults == 0
> >> handling? As it stands, this code can't be reached.
> >>
> >> I don't mind keeping it as-is if you prefer though. I just found it a
> >> little hard to follow in this context.
> > You're right that this and patch 8 are really intertwined. I developed
> > them simultaneously and didn't want to split up this logic as it would
> > have involved writing new logic to delete later.
> >
> > I can do that if you would like. But I'd rather not personally :)>
>
> No, no need to add temporary logic. But just so I understand:
> I was thinking that the num_defaults == 0 paths would be purely
> additive. Is that not the case?
Ah no you're correct, it is purely addetive here. It is only
used from patch 5 onwards.
Appologies, my memory of a lot of this logic isn't very fresh.
>
> Richard
--
Alfie Richards
