On Mon, 4 Nov 2024, alfie.richa...@arm.com wrote:
> /* 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. */
Note typos in this comment: "teh", "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.
// 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.
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.
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.
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.
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.)?
--
Joseph S. Myers
josmy...@redhat.com
