Adding missing CC's
-------- Forwarded Message --------
Subject: Re: [PATCH v3] C: Support Function multiversionsing in the C
front end
Date: Mon, 11 Nov 2024 12:29:57 +0000
From: Alfie Richards <alfie.richa...@arm.com>
To: Joseph Myers <josmy...@redhat.com>
Hi Joseph,
Thank you for the detailed feedback. I am quite junior and appreciate your
patience with some details of this and with the more amateurish mistakes.
/* Subroutine of duplicate_decls. Compare NEWDECL to OLDDECL.
Returns true if the caller should proceed to merge the two, false
if OLDDECL should simply be discarded. As a side effect, issues
@@ -3365,11 +3382,53 @@ pushdecl (tree x)
b->inner_comp = false;
b_use = b;
b_ext = b;
+
+ /* Check if the function is part of a group of multiversioned
functions.
+ If so, we should check for conflicts or duplicates between any with
+ the same version. If there are no conflicts, should just add the new
+ binding. */
+ if (b && TREE_CODE (x) == FUNCTION_DECL
+ && TREE_CODE (b->decl) == FUNCTION_DECL
+ && (DECL_FUNCTION_VERSIONED (b->decl)
+ || targetm.target_option.function_versions (x, b->decl)))
+ {
+ maybe_mark_function_versioned (x);
+ for (; b_use; b_use = b_use->shadowed)
+ {
+ if (!comptypes (type, TREE_TYPE (b_use->decl))
+ || TREE_CODE (b_use->decl) != FUNCTION_DECL)
+ {
+ /* Found a conflicting declaration.
+ duplicate_decls will create teh diagnostics forthis. */
+ b = b_use;
+ break;
I don't follow the logic you're using here. This loop is going through
all bindings for the name in containing scopes, including completely
unrelated ones such as object declarations with block scope and automatic
storage duration, which looks like it would do the wrong thing.
I see this code is very unclear in hindsight. The logic of this code
relies on that FMV functions are only allowed at file scope.
This should have a DECL_FILE_SCOPE_P check to avoid some of the ridiculous
cases you mentioned.
The code is intended to iterate through the bindings to `foo` at this top
level and accomplish:
- Building the cgraph for the FMV functions
- Noticing if any of the bindings are completely different,
or have conflicting signature and in that case falling through to the
usual pushdecl logic to either allow this (in the case of higher
scope) or
emit diagnostics for this case
- If it finds two declarations with the same target, then running the usual
pushdecl logic to merge these or emit diagnostics if they conflict
- If this declaration has a new target that hasn't been seen before, skip
the pushdecl logic (as there is no previous declaration for this to be
merged with and, by the above, we know its compatible) and
jump forward to adding the new binding shadowing the current one
(so that this decl can be iterated over by future bindings).
In this way I suppose I'm sort of using the "shadows" structure to be a
linked
list of my FMV bindings, to link them up and work out the different
versions.
I have asked the ACLE folks to consider explicitly adding the requirement
that all FMV functions are at the file scope level.
// File scope
__attribute__ ((target_version ("default"))) int foo ();
{
// Block scope
int foo;
{
__attribute__ ((target_version ("default"))) int foo ();
__attribute__ ((target_version ("default"))) int foo ();
As I read this code, if foo is a multiversioned function then it would
find the variable declaration in the intermediate scope and treat it as
conflicting. Compare the existing logic looping through scopes for
external declarations, which is careful to ignore any declarations not
satisfying DECL_FILE_SCOPE_P.
I agree the current logic is broken for this case. I think that by adding
a check that the relevant decls are at File scope should avoid such cases?
There is a lot of complicated logic in pushdecl that it seems a bad idea
to bypass for multiversioned functions. For example, everything related
to putting declarations with external linkage in the "external scope" in
addition to their main scope.
Yes, I agree, pushdecl seems very intricate and I aimed in this patch
to not interfere with is as much as possible as I am aware I have too
light an understanding of the C language to understand all the cases and
considerations :)
The skip over pushdecl logic was only intended for the case when processing
a new target version and is mostly based off following the code path of when
duplicate_decl returns false (which I believe is the case where they are
not conflicting?).
I think a patch like this needs to come with a clearly stated semantic
model for how the multiversioned functions are meant to be represented
inside the front end, and how that translates into consequences for
declaration processing, against which the patch can be tested. I suggest
some principles like this:
* For the purposes of*checking compatible types*, all declarations of the
same name - different versions and the dispatcher - should be compared and
required to be consistent. Based on the tests you have, it appears at
least some of this is taking place - for example, when two declarations
have incompatible types.
* For the purposes of*combining information from multiple declarations*,
at least some of that should happen for different versions as well. For
example, if one version has a parameter int (*x)[] and another has a
parameter int (*x)[2], the composite type should be formed and used for
both versions.
* For the purposes of*merging declarations and discarding the new one*,
however, only declarations of the same version should be merged.
Yes agreed. For the next patch I will add some more tests for these cases
and a clearer description of the semantic model. I believe my current code
makes an attempt at point 1 (which I will improve) and fully does point 3.
For point 2 I hadn't considered this. I have reached out to the ACLE folks
to get clarification on what the correct behaviour is. It currently
stipulates
All instances of the versions shall share the same function signature
and calling convention.
But I am not certain where the line between "different signatures"
and "providing more information but still the same signature" is.
I think we will do as you suggest and merge the information for all of
them and I don't believe that would be too hard to add to my current code.
So all effects of diagnose_mismatched_decls should apply between different
versions. But only a subset of merge_decls is relevant (such as forming
composite types) - and that subset would be relevant in both directions,
because both declarations remain in use afterwards. Or if you don't want
the complications there, you could define the semantics not to e.g. form
composite types between different versions.
I don't follow what you mean that the logic of diagnose_mismatched_decls
should apply between versions? Shouldn't diagnose_mismatched_decls
always produce redefinition diagnostics in these cases as it isn't aware
of FMV attributes? Are you referring specifically to the logic for
combining signatures?
Given these principles, pretty much all existing logic in pushdecl is
relevant for multiversioned functions - twice over. It's fully relevant
for matching and merging with other declarations for the same version of
the function (but with all the logic that looks for bindings for a given
symbol name needing to be adjusted to consider only those for the same
version as potentially matching / needing to be merged). And it's mostly
relevant for declarations for other versions of the function, to the
extent that type compatibility needs to be checked and maybe composite
types formed.
But maybe we need to start the conceptual design at an earlier stage.
In the presence of multiple declarations of different versions of a
function, (a) what DECL or DECLs should be recorded as bindings for the
function name in the scope in which the declaration appears, and (b) what
DECL or DECLs should be present in the IR at all? Note that e.g.
lookup_name definitely expects only one binding for a given name (as an
ordinary identifier) in a given scope, so it can only find one of the
declarations. So any answer you give needs to include also a sensible
conclusion on (c) what DECL should be returned by functions such as
lookup_name?
Maybe only the main (dispatcher?) declaration for a multiversioned
function should appear in scopes and pass through pushdecl, and that
declaration should then in turn link in some way to the declarations for
the other versions? If that's the design, then each time a declaration
for a particular version is encountered, it should somehow generate (a) a
new DECL for the dispatcher, which is passed through pushdecl to deal with
type compatibility checking, composite type formation and ensuring
appropriate bindings are present in each scope, and (b) if the previous
checks pass, then an internal DECL for the actual version being declared,
which would not appear in any scope, but would still get merged with any
previous declaration for that version (for composite types, merging
attributes, etc.)?
I agree that it may be better to take a different approach.
There are a couple considerations that I am worried about though for
any new approach:
- a TU can have just a versioned function, and no default or other versions
in which case it should be correctly mangled and ready for linking
(this isn't done in this patch, see my second patch which changes this
behaviour for AArch64)
- an unannotated function and a function annotated "default" should behave
identically. This makes things slightly difficult as when processing
a default function as you may later find out that it is the default
version of
a multiversioned function.
If we take the approach you suggested, I think this would then mean
removing the binding later and adding the dispatcher? I also then imagine
that in turn requires rewriting the calls to the function?
- as much as possible I would like to stay as close to how the C++
frontend handles this to avoid having to mess around with the hooks that
I am sharing between them. I think this change would remove the call
redirection
at the gimplify stage which, while I don't yet see any reason why
this would
change the hooks, makes me uncomfortable at the difference in the
methods.
I should also add: the ACLE specification for the details of how function
multiversioning is supposed to work in terms of interactions of
declarations for different versions in the same or different scopes and
what happens regarding forming composite types seems rather vague. So
maybe it would be a good idea to clarify the specification for what the
intended semantics actually are in such cases.
I've asked them to add more clarity to be provided for this.
I am happy to go either way on this. Either, I can convince you this current
logic is relatively sane, add logic to update signature information between
target versions and the dispatcher decl, check top level declarations, and
add more tests for many more tests of these cases.
Or I can have a rethink.
I think there may be a cleaner way to do this, especially as the cgraph
structure is there and could be used for this.
However, I worry it would require quite a sizable rewrite of some of the
logic in pushdecl/diagnose_mismatched/mergedecl to be more modularized
so they could be used in their existing situations and also repurposed for
the cases I need it for also.
Kind regards,
Alfie Richards
