diff mbox series

[v2,3/5] ada: Improve attribute exclusion handling

Message ID 10532e77-2eb3-5043-0b71-faf415c5a1af@e124511.cambridge.arm.com
State Accepted
Headers
Series target_version and aarch64 function multiversioning |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Andrew Carlotti Nov. 17, 2023, 2:54 a.m. UTC 
  Change the handling of some attribute mutual exclusions to use the
generic attribute exclusion lists, and fix some asymmetric exclusions by
adding the exclusions for always_inline after noinline or target_clones.

Aside from the new always_inline exclusions, the only change is
functionality is the choice of warning message displayed.  All warnings
about attribute mutual exclusions now use the same message.

---

I haven't manged to test the Ada frontend, but this patch (and the following
one) contain only minimal change to functionality, which I have tested by
copying the code to the C++ frontend and verifying the behaviour of equivalent
changes there.  Is this ok to push without further testing?  If not, then could
someone test this series for me?

gcc/ada/ChangeLog:

	* gcc-interface/utils.cc (attr_noinline_exclusions): New.
	(attr_always_inline_exclusions): Ditto.
	(attr_target_exclusions): Ditto.
	(attr_target_clones_exclusions): Ditto.
	(gnat_internal_attribute_table): Add new exclusion lists.
	(handle_noinline_attribute): Remove custom exclusion handling.
	(handle_target_attribute): Ditto.
	(handle_target_clones_attribute): Ditto.

Comments

Marc Poulhiès Nov. 17, 2023, 10:45 a.m. UTC | #1 
Hello,

> I haven't manged to test the Ada frontend, but this patch (and the following

I don't have an aarch64 setup to test, but I may be able to help with the
issue preventing you from testing. Can you elaborate what is the problem?

Marc
Andrew Carlotti Nov. 17, 2023, 11:15 a.m. UTC | #2 
On Fri, Nov 17, 2023 at 11:45:16AM +0100, Marc Poulhi�s wrote:
> 
> Hello,
> 
> > I haven't manged to test the Ada frontend, but this patch (and the following
> 
> I don't have an aarch64 setup to test, but I may be able to help with the
> issue preventing you from testing. Can you elaborate what is the problem?
> 
> Marc

I only really got as far as trying to configure a build environemnt, which
failed with 'configure: error: GNAT is required to build ada'.  I have no prior
Ada experience, and I couldn't work out how to get any relevant test code to
compile on Compiler Explorer.  I therefore decided it wasn't worth me spending
more effort trying to test from Ada a small change to some code that is
effectively front-end independent, but just happens to be added to a limited
subset of front ends.

It's probably sufficient to simply test that the Ada changes can be built for
any target, since I'd be surprised if I've managed to copy this code from C++
in a way that breaks functionality without obviously breaking the build.
Marc Poulhiès Nov. 20, 2023, 8:26 a.m. UTC | #3 
Andrew Carlotti <andrew.carlotti@arm.com> writes:

> On Fri, Nov 17, 2023 at 11:45:16AM +0100, Marc Poulhi�s wrote:
>>
>> Hello,
>>
>> > I haven't manged to test the Ada frontend, but this patch (and the following
>>
>> I don't have an aarch64 setup to test, but I may be able to help with the
>> issue preventing you from testing. Can you elaborate what is the problem?
>>
>> Marc
>
> I only really got as far as trying to configure a build environemnt, which
> failed with 'configure: error: GNAT is required to build ada'.  I have no prior
> Ada experience, and I couldn't work out how to get any relevant test code to
> compile on Compiler Explorer.  I therefore decided it wasn't worth me spending
> more effort trying to test from Ada a small change to some code that is
> effectively front-end independent, but just happens to be added to a limited
> subset of front ends.
>
> It's probably sufficient to simply test that the Ada changes can be built for
> any target, since I'd be surprised if I've managed to copy this code from C++
> in a way that breaks functionality without obviously breaking the build.

Hello,

I've tested your changes. The compiler builds correctly and there's
no regression (x86_64-linux) + I've also executed some extra tests.

> gcc/ada/ChangeLog:
> 	* gcc-interface/utils.cc (attr_noinline_exclusions): New.
> 	(attr_always_inline_exclusions): Ditto.
> 	(attr_target_exclusions): Ditto.
> 	(attr_target_clones_exclusions): Ditto.
> 	(gnat_internal_attribute_table): Add new exclusion lists.
> 	(handle_noinline_attribute): Remove custom exclusion handling.
> 	(handle_target_attribute): Ditto.
> 	(handle_target_clones_attribute): Ditto.

Ok.

Marc
diff mbox series

Patch

diff --git a/gcc/ada/gcc-interface/utils.cc b/gcc/ada/gcc-interface/utils.cc
index 8b2c7f99ef3060603658e438b71a3bfa3ef7f2ac..e33a63948cebdeafc3abcdd539a35141969ad978 100644
--- a/gcc/ada/gcc-interface/utils.cc
+++ b/gcc/ada/gcc-interface/utils.cc
@@ -130,6 +130,32 @@  static const struct attribute_spec::exclusions attr_stack_protect_exclusions[] =
   { NULL, false, false, false },
 };
 
+static const struct attribute_spec::exclusions attr_always_inline_exclusions[] =
+{
+  { "noinline", true, true, true },
+  { "target_clones", true, true, true },
+  { NULL, false, false, false },
+};
+
+static const struct attribute_spec::exclusions attr_noinline_exclusions[] =
+{
+  { "always_inline", true, true, true },
+  { NULL, false, false, false },
+};
+
+static const struct attribute_spec::exclusions attr_target_exclusions[] =
+{
+  { "target_clones", true, true, true },
+  { NULL, false, false, false },
+};
+
+static const struct attribute_spec::exclusions attr_target_clones_exclusions[] =
+{
+  { "always_inline", true, true, true },
+  { "target", true, true, true },
+  { NULL, false, false, false },
+};
+
 /* Fake handler for attributes we don't properly support, typically because
    they'd require dragging a lot of the common-c front-end circuitry.  */
 static tree fake_attribute_handler (tree *, tree, tree, int, bool *);
@@ -165,7 +191,7 @@  const struct attribute_spec gnat_internal_attribute_table[] =
   { "strub",	    0, 1, false, true, false, true,
     handle_strub_attribute, NULL },
   { "noinline",     0, 0,  true,  false, false, false,
-    handle_noinline_attribute, NULL },
+    handle_noinline_attribute, attr_noinline_exclusions },
   { "noclone",      0, 0,  true,  false, false, false,
     handle_noclone_attribute, NULL },
   { "no_icf",       0, 0,  true,  false, false, false,
@@ -175,7 +201,7 @@  const struct attribute_spec gnat_internal_attribute_table[] =
   { "leaf",         0, 0,  true,  false, false, false,
     handle_leaf_attribute, NULL },
   { "always_inline",0, 0,  true,  false, false, false,
-    handle_always_inline_attribute, NULL },
+    handle_always_inline_attribute, attr_always_inline_exclusions },
   { "malloc",       0, 0,  true,  false, false, false,
     handle_malloc_attribute, NULL },
   { "type generic", 0, 0,  false, true,  true,  false,
@@ -192,9 +218,9 @@  const struct attribute_spec gnat_internal_attribute_table[] =
   { "simd",         0, 1,  true,  false, false, false,
     handle_simd_attribute, NULL },
   { "target",       1, -1, true,  false, false, false,
-    handle_target_attribute, NULL },
+    handle_target_attribute, attr_target_exclusions },
   { "target_clones",1, -1, true,  false, false, false,
-    handle_target_clones_attribute, NULL },
+    handle_target_clones_attribute, attr_target_clones_exclusions },
 
   { "vector_size",  1, 1,  false, true,  false, false,
     handle_vector_size_attribute, NULL },
@@ -6742,16 +6768,7 @@  handle_noinline_attribute (tree *node, tree name,
 			   int ARG_UNUSED (flags), bool *no_add_attrs)
 {
   if (TREE_CODE (*node) == FUNCTION_DECL)
-    {
-      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (*node)))
-	{
-	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
-		   "with attribute %qs", name, "always_inline");
-	  *no_add_attrs = true;
-	}
-      else
-	DECL_UNINLINABLE (*node) = 1;
-    }
+    DECL_UNINLINABLE (*node) = 1;
   else
     {
       warning (OPT_Wattributes, "%qE attribute ignored", name);
@@ -7050,12 +7067,6 @@  handle_target_attribute (tree *node, tree name, tree args, int flags,
       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_attribute_p (*node, name, args, flags))
     *no_add_attrs = true;
 
@@ -7083,23 +7094,8 @@  handle_target_clones_attribute (tree *node, tree name, tree ARG_UNUSED (args),
 {
   /* Ensure we have a function type.  */
   if (TREE_CODE (*node) == FUNCTION_DECL)
-    {
-      if (lookup_attribute ("always_inline", DECL_ATTRIBUTES (*node)))
-	{
-	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
-		   "with %qs attribute", name, "always_inline");
-	  *no_add_attrs = true;
-	}
-      else if (lookup_attribute ("target", DECL_ATTRIBUTES (*node)))
-	{
-	  warning (OPT_Wattributes, "%qE attribute ignored due to conflict "
-		   "with %qs attribute", name, "target");
-	  *no_add_attrs = true;
-	}
-      else
-	/* Do not inline functions with multiple clone targets.  */
-	DECL_UNINLINABLE (*node) = 1;
-    }
+    /* Do not inline functions with multiple clone targets.  */
+    DECL_UNINLINABLE (*node) = 1;
   else
     {
       warning (OPT_Wattributes, "%qE attribute ignored", name);