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.
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 :)
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.
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.
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.
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
