From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id C6AD33858C20 for ; Sun, 5 Nov 2023 20:18:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C6AD33858C20 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=arm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org C6AD33858C20 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=217.140.110.172 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699215540; cv=none; b=lCFbj08H5/VQwgtyo3REAgm3ELNLB8JJdvwCSpZrRMqpzsjTDqazOB+Spke2ezO1VKpO1/KJaYy4AaE3KlWFSlfQTEvhm9NYR2rpakALaP9H2+oHuOep+8DIH9rKd2zTurek7/SEsd7jyUwZmPB532T+Nmv65RQ66sQnh12w2jo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699215540; c=relaxed/simple; bh=LdC3VqmztvMSpN4VhUrQ9RapBnd5lRtwPRy6D5LFdLU=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=mbioOyvY/6D97Dnpowwhw0QErOQaU9pcjXicG4nwPf3P97PYFm4kDrPVbPfk8JqUjaVvcwkyzgBd5ZwXwQ28KVW2tghcN7A0j/pY6r0IjBKz++MeBWV++6JzHhAkidgEGAsyCOidDiWZRzrHvv7Tkm4huXCT14A4ktn/OyeNWiE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 38B60C15; Sun, 5 Nov 2023 12:19:42 -0800 (PST) Received: from localhost (e121540-lin.manchester.arm.com [10.32.110.72]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 8E7B83F703; Sun, 5 Nov 2023 12:18:57 -0800 (PST) From: Richard Sandiford To: Andrew Carlotti Mail-Followup-To: Andrew Carlotti ,gcc-patches@gcc.gnu.org, jason@redhat.com, nathan@acm.org, rguenther@suse.de, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, jason@redhat.com, nathan@acm.org, rguenther@suse.de Subject: Re: [1/3] Add support for target_version attribute References: <26bbc7e4-9d5a-fef3-2f78-1b7a03865050@e124511.cambridge.arm.com> <2a9e5b6e-9720-602b-5449-28fb5d88a40c@e124511.cambridge.arm.com> <75536b23-106a-89a4-7c17-41176c868de7@e124511.cambridge.arm.com> Date: Sun, 05 Nov 2023 20:18:56 +0000 In-Reply-To: <75536b23-106a-89a4-7c17-41176c868de7@e124511.cambridge.arm.com> (Andrew Carlotti's message of "Fri, 3 Nov 2023 12:43:35 +0000") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Spam-Status: No, score=-23.3 required=5.0 tests=BAYES_00,GIT_PATCH_0,KAM_DMARC_NONE,KAM_DMARC_STATUS,KAM_LAZY_DOMAIN_SECURITY,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: Andrew Carlotti writes: > On Thu, Oct 26, 2023 at 07:41:09PM +0100, Richard Sandiford wrote: >> Andrew Carlotti writes: >> > This patch adds support for the "target_version" attribute to the middle >> > end and the C++ frontend, which will be used to implement function >> > multiversioning in the aarch64 backend. >> > >> > Note that C++ is currently the only frontend which supports >> > multiversioning using the "target" attribute, whereas the >> > "target_clones" attribute is additionally supported in C, D and Ada. >> > Support for the target_version attribute will be extended to C at a >> > later date. >> > >> > Targets that currently use the "target" attribute for function >> > multiversioning (i.e. i386 and rs6000) are not affected by this patch. >> > >> > >> > I could have implemented the target hooks slightly differently, by reusing the >> > valid_attribute_p hook and adding attribute name checks to each backend >> > implementation (c.f. the aarch64 implementation in patch 2/3). Would this be >> > preferable? >> >> Having as much as possible in target-independent code seems better >> to me FWIW. On that basis: >> >> > >> > Otherwise, is this ok for master? >> > >> > >> > gcc/c-family/ChangeLog: >> > >> > * c-attribs.cc (handle_target_version_attribute): New. >> > (c_common_attribute_table): Add target_version. >> > (handle_target_clones_attribute): Add conflict with >> > target_version attribute. >> > >> > gcc/ChangeLog: >> > >> > * attribs.cc (is_function_default_version): Update comment to >> > specify incompatibility with target_version attributes. >> > * cgraphclones.cc (cgraph_node::create_version_clone_with_body): >> > Call valid_version_attribute_p for target_version attributes. >> > * target.def (valid_version_attribute_p): New hook. >> > (expanded_clones_attribute): New hook. >> > * doc/tm.texi.in: Add new hooks. >> > * doc/tm.texi: Regenerate. >> > * multiple_target.cc (create_dispatcher_calls): Remove redundant >> > is_function_default_version check. >> > (expand_target_clones): Use target hook for attribute name. >> > * targhooks.cc (default_target_option_valid_version_attribute_p): >> > New. >> > * targhooks.h (default_target_option_valid_version_attribute_p): >> > New. >> > * tree.h (DECL_FUNCTION_VERSIONED): Update comment to include >> > target_version attributes. >> > >> > gcc/cp/ChangeLog: >> > >> > * decl2.cc (check_classfn): Update comment to include >> > target_version attributes. >> > >> > >> > diff --git a/gcc/attribs.cc b/gcc/attribs.cc >> > index b1300018d1e8ed8e02ded1ea721dc192a6d32a49..a3c4a81e8582ea4fd06b9518bf51fad7c998ddd6 100644 >> > --- a/gcc/attribs.cc >> > +++ b/gcc/attribs.cc >> > @@ -1233,8 +1233,9 @@ make_dispatcher_decl (const tree decl) >> > return func_decl; >> > } >> > >> > -/* Returns true if decl is multi-versioned and DECL is the default function, >> > - that is it is not tagged with target specific optimization. */ >> > +/* 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. */ >> > >> > bool >> > is_function_default_version (const tree decl) >> > diff --git a/gcc/c-family/c-attribs.cc b/gcc/c-family/c-attribs.cc >> > index 072cfb69147bd6b314459c0bd48a0c1fb92d3e4d..1a224c036277d51ab4dc0d33a403177bd226e48a 100644 >> > --- a/gcc/c-family/c-attribs.cc >> > +++ b/gcc/c-family/c-attribs.cc >> > @@ -148,6 +148,7 @@ static tree handle_alloc_align_attribute (tree *, tree, tree, int, bool *); >> > static tree handle_assume_aligned_attribute (tree *, tree, tree, int, bool *); >> > static tree handle_assume_attribute (tree *, tree, tree, int, bool *); >> > static tree handle_target_attribute (tree *, tree, tree, int, bool *); >> > +static tree handle_target_version_attribute (tree *, tree, tree, int, bool *); >> > static tree handle_target_clones_attribute (tree *, tree, tree, int, bool *); >> > static tree handle_optimize_attribute (tree *, tree, tree, int, bool *); >> > static tree ignore_attribute (tree *, tree, tree, int, bool *); >> > @@ -480,6 +481,8 @@ const struct attribute_spec c_common_attribute_table[] = >> > handle_error_attribute, NULL }, >> > { "target", 1, -1, true, false, false, false, >> > handle_target_attribute, NULL }, >> > + { "target_version", 1, -1, true, false, false, false, >> > + handle_target_version_attribute, NULL }, >> > { "target_clones", 1, -1, true, false, false, false, >> > handle_target_clones_attribute, NULL }, >> > { "optimize", 1, -1, true, false, false, false, >> > @@ -5569,6 +5572,45 @@ handle_target_attribute (tree *node, tree name, tree args, int flags, >> > return NULL_TREE; >> > } >> > >> > +/* Handle a "target_version" attribute. */ >> > + >> > +static tree >> > +handle_target_version_attribute (tree *node, tree name, tree args, int flags, >> > + bool *no_add_attrs) >> > +{ >> > + /* Ensure we have a function type. */ >> > + if (TREE_CODE (*node) != FUNCTION_DECL) >> > + { >> > + warning (OPT_Wattributes, "%qE attribute ignored", name); >> > + *no_add_attrs = true; >> > + } >> > + else if (lookup_attribute ("target_clones", DECL_ATTRIBUTES (*node))) >> > + { >> > + warning (OPT_Wattributes, "%qE attribute ignored due to conflict " >> > + "with %qs attribute", name, "target_clones"); >> > + *no_add_attrs = true; >> > + } >> > + else if (!targetm.target_option.valid_version_attribute_p (*node, name, args, >> > + flags)) >> > + *no_add_attrs = true; >> > + >> > + /* Check that there's no empty string in values of the attribute. */ >> > + for (tree t = args; t != NULL_TREE; t = TREE_CHAIN (t)) >> > + { >> > + tree value = TREE_VALUE (t); >> > + if (TREE_CODE (value) == STRING_CST >> > + && TREE_STRING_LENGTH (value) == 1 >> > + && TREE_STRING_POINTER (value)[0] == '\0') >> > + { >> > + warning (OPT_Wattributes, >> > + "empty string in attribute %"); >> > + *no_add_attrs = true; >> > + } >> > + } >> >> would it make sense to do the empty string test first, and only pass >> the vetted arguments to the target hook? Also, a Google search suggests >> that there aren't any pre-existing, conflicting uses of "target_version" >> that take multiple arguments. So could this code check that there >> is exactly one argument (by changing 1, -1 to 1, 1 in the spec above) >> and then require it to be a nonempty string? It could then pass the >> string itself to the target hook (probably as a const char *). >> >> (FWIW, it doesn't look like the Clang documentation has kept the door >> open to multiple arguments.) >> >> I wonder if we could use attribute_spec::exclusions to describe the >> mutual exclusion with "target_clones". It doesn't look like the >> existing code does, though, so maybe not. >> >> I couldn't see anything that forbids a combination of "target" and >> "target_version". Should that combination be allowed? In some ways >> it makes conceptual sense, since using "target" is like changing the >> command-line options. But I suppose we'd then need to diagnose conflicts >> and deal with ordering issues. So perhaps "target" should be made >> mutually exclusive as well. > > My aarch64 backend pass deals with backend issues by always applying > target_version attribute changes after target_attribute changes. I don't think > there's any additional conflicts to worry about, since adding a target_version > is simply equivalent to enabling extra features in the target string. > > A similar thing would work for target_clones, but I didn't initially do that > because it would require making the frontend exclusions target dependant. > However, I think it's just a case of checking the backend hook to see whether > target_clones gets expanded to target attributes or not. > > Clang currently disallows combining target attributes with either > target_version or target_clones. However, I think it's worth being able to > combine these attributes. For example, it could be useful to use the target > attribute to select different tuning for an sve target_version. Ah, ok, thanks for the explanation. That approach sounds good to me. Richard