Alfie Richards <alfie.richa...@arm.com> writes:
> Hi Richard,
>
> Thank you for the feedback!
>
> On 10/12/2024 18:49, Richard Sandiford wrote:
>>> -/* Returns true if DECL is multi-versioned using the target attribute, and
>>> this
>>> - is the default version. This function can only be used for targets
>>> that do
>>> - not support the "target_version" attribute. */
>>> +/* Returns true if DECL could be a default version of a FMV function set
>>> + If TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_DECL is false, this is when
>>> DECL
>> TARGET_CREATE_FMV_DISPATCHER_AT_DEFAULT_IMPL
>>
>> But could we key this off !TARGET_HAS_FMV_TARGET_ATTRIBUTE instead?
>> At the moment that conditino is essentially "aarch64 semantics", and given
>> the general complexity of FMV, it would be good to avoid creating more
>> variations than we support.
>
> This function has always seemed like a bad compromise to me and is
> currently trying to answer two different questions.
>
> I don't think having it depend on TARGET_HAS_FMV_TARGET_ATTRIBUTE is
> necessarily any better? Both are false equivalences in my opinion.
>
> I think I would prefer to create a different function called
> "could_function_be_default_version" containing the different semantics
> which could more reasonably be conditioned on
> TARGET_HAS_FMV_TARGET_ATTRIBUTE.
> Then similarly, is_function_default_version can be changed back to
> working out if a function definitely is the default version
I'm not sure. The AArch64 semantics seem to be that an explicit
target_version("default") is usually equivalent to no attribute.
The only potential difference I'm aware of is whether:
__attribute__((target_version("default"))) void f(void) {}
should force a .default alias to be created even when f resolves directly
to the function. (I'm asking internally for clarification on that.)
So would the distinction between "is" and "could be" be useful in
practice? This predicate looks at individual decls, whereas the AArch64
semantics mean that "is" is instead a property of the TU as a whole.
>> AIUI, we rely on get_function_versions_dispatcher to arrange this ordering.
>> (Each target does it independently, sigh.) Can we rely on the hook's
>> having been called at this point?
>>
>> That is, it seems like this code assumes that
>> get_function_versions_dispatcher has already been called, whereas the
>> code below calls get_function_versions_dispatcher on-the-fly when it
>> sees the default version. Does something guarantee that the default
>> node is analysed before all of the other versions?
> Ahh fair point. I am convinced we can at this point certainly, as this case
> is only
> triggered if this is the dispatched symbol, which is created by that function
> :)
Oops, yes. I thought at first it was keyed off non-default versions.
> It is a good point in general and there may be other cases I've made the
> assumption that this is true.
>
> In my other patch, I change the logic for record_function_versions
> to implicitly ensure the default is always the first version in any
> fmv structure, which I think is a good change and I think I will split it
> off into a separate patch for both of these to rely on.
Sounds good if I've understood correctly.
>> The convention is for error messages to start with lower case and not
>> to end with ".".
> Thank you! I will change this.
>> There should be a test to exercise this codepath. Locally I tried:
>>
>> __attribute__((target_version("sve"))) int f() { return 2; }
>> int g() { return f(); }
>>
>> but it seems to ICE in aarch64_get_function_versions_dispatcher.
>> (Hope I didn't mess up the testing.)
> Oh dear, apologies. I think I had created tests for this in the
> C frontend as that was more my focus when creating this, and clearly
> didn't investigate the C++ versions thoroughly enough. I will
> copy my tests there over and make sure these paths and cases work
> correctly.
>> As discussed off-list, this was a last-minute change that doesn't
>> compile 🙂 The earlier:
>>
>> /* Mark the assembler name as set to prevent it getting mangled again
>> .*/
>> SET_DECL_ASSEMBLER_NAME (dispatch_decl,
>> DECL_ASSEMBLER_NAME (dispatch_decl));
>>
>> is probably correct, but I think it'd be worth moving into
>> make_dispatcher_decl, for !TARGET_HAS_FMV_TARGET_ATTRIBUTE.
>>
>> PRs 117987 and 117989 suggest that, independently of this patch,
>> we seem to have some issues around FMV and assembler names.
> Yes apologies for the unforced error! I think I was in the wrong work
> tree or checkout.
>
> I agree it is better in make_dispatcher_decl but I was a little hesitant
> to change other back ends behaviour. Specifically I believe x86 exhibits
> the double mangling behaviour at the moment and was nervous to change that as
> it could be a breaking change for them.
Yeah, that's why I was suggesting keying it off
!TARGET_HAS_FMV_TARGET_ATTRIBUTE (aka the "AArch64 semantics" toggle).
That should ensure that other targets are unaffected.
>
> I would appreciate any advice from your experience with how to handle such
> things.
>> Could you explain why this is needed in more detail? I assume the
>> premise of the old code was that we didn't need to call
>> cgraph_node::record_function_versions if both functions were already
>> marked as versioned.
>>
>> Admittedly, I don't know why that seemed necessary, since
>> cgraph_node::record_function_versions itself has an early-out.
>> But it sounded like this hunk was needed for correctness, rather than
>> simply being a clean-up.
> Basically just as you say. It seemed like this was an argument here to prevent
> the recording, and it was always calculated by checking if the arguments
> had already been recorded. But, the record_function_versions function
> does that exact check already.
>
> Additionally, as I could no longer rely on DECL_FUNCTION_VERSIONED to
> imply that a function had been recorded (as this patch means functions
> get marked as versioned much earlier (see the start_preparsed_function
> change)).
> Initially I created a new flag to replace this to imply if a function had been
> recorded, before realising the whole thing was unnecessary so removed it all.
>
> I can't any imagine any situation we would want to mark functions and not
> record
> them in an FMV structure, and couldn't find any cases in code where we do that
> so I think this is fine.
OK, thanks, makes sense. If we're confident that this part is a no-op,
it might be worth splitting it out into a pre-patch, partly for review
purposes and partly for bisection.
Richard
>
> Thanks for the feedback and your time!
>
> My current plan is to spin off a new patch covering the change to record
> function
> version that keeps the default as the first argument and adds "could function
> be
> default" function (as I think that's the cleanest solution right now) and then
> subsequently clean-up this patch with your feedback on top of that.
>
> Kind regards,
> Alfie
