[1/2] Fortran: Cleanup struct ext_attr_t

Message ID 20221110102031.1366016-2-aldot@gcc.gnu.org
State Unresolved
Headers
Series Fortran: Add attribute flatten |

Checks

Context Check Description
snail/gcc-patch-check warning Git am fail log

Commit Message

Bernhard Reutner-Fischer Nov. 10, 2022, 10:20 a.m. UTC
  Tiny cleanup opportunity since we now have ext_attr_args in
struct symbol_attribute.
Bootstrapped and regtested on x86_64-unknown-linux with no new
regressions.
Ok for trunk if the prerequisite was approved ([PATCH 2/2] Fortran: add
attribute target_clones) ?

gcc/fortran/ChangeLog:

	* gfortran.h (struct ext_attr_t): Remove middle_end_name.
	* trans-decl.cc (add_attributes_to_decl): Move building
	tree_list to ...
	* decl.cc (gfc_match_gcc_attributes): ... here. Add the attribute to
	the tree_list for the middle end.

Cc: gfortran ML <fortran@gcc.gnu.org>
---
 gcc/fortran/decl.cc       | 35 +++++++++++++++++++++++------------
 gcc/fortran/gfortran.h    |  1 -
 gcc/fortran/trans-decl.cc | 13 +------------
 3 files changed, 24 insertions(+), 25 deletions(-)
  

Comments

Mikael Morin Nov. 21, 2022, 11:08 a.m. UTC | #1
Hello,

Le 10/11/2022 à 11:20, Bernhard Reutner-Fischer via Fortran a écrit :
> Tiny cleanup opportunity since we now have ext_attr_args in
> struct symbol_attribute.
> Bootstrapped and regtested on x86_64-unknown-linux with no new
> regressions.
> Ok for trunk if the prerequisite was approved ([PATCH 2/2] Fortran: add
> attribute target_clones) ?
> 
> gcc/fortran/ChangeLog:
> 
> 	* gfortran.h (struct ext_attr_t): Remove middle_end_name.
> 	* trans-decl.cc (add_attributes_to_decl): Move building
> 	tree_list to ...
> 	* decl.cc (gfc_match_gcc_attributes): ... here. Add the attribute to
> 	the tree_list for the middle end.
> 
I prefer to not do any middle-end stuff at parsing time, so I would 
rather not do this change.
Not OK.
  
Bernhard Reutner-Fischer Nov. 21, 2022, 8:34 p.m. UTC | #2
On Mon, 21 Nov 2022 12:08:20 +0100
Mikael Morin <morin-mikael@orange.fr> wrote:

> > 	* gfortran.h (struct ext_attr_t): Remove middle_end_name.
> > 	* trans-decl.cc (add_attributes_to_decl): Move building
> > 	tree_list to ...
> > 	* decl.cc (gfc_match_gcc_attributes): ... here. Add the attribute to
> > 	the tree_list for the middle end.
> >   
> I prefer to not do any middle-end stuff at parsing time, so I would 
> rather not do this change.
> Not OK.

Ok, that means we should filter-out those bits that we don't want to
write to the module (right?). We've plenty of bits left, more than Dave
Love would want to have added, i hope, so that should not be much of a
concern.

What that table really wants to say is whether or not this attribute
should be passed to the ME. Would it be acceptable to remove these
duplicate strings and just have a bool/char/int that is true if it
should be lowered (in trans-decl, as before)? But now i admit it's just
bikeshedding and we can as well leave it alone for now.. It was just a
though.

thanks,
  
Mikael Morin Nov. 22, 2022, 11:52 a.m. UTC | #3
Le 21/11/2022 à 21:34, Bernhard Reutner-Fischer a écrit :
> On Mon, 21 Nov 2022 12:08:20 +0100
> Mikael Morin <morin-mikael@orange.fr> wrote:
> 
>>> 	* gfortran.h (struct ext_attr_t): Remove middle_end_name.
>>> 	* trans-decl.cc (add_attributes_to_decl): Move building
>>> 	tree_list to ...
>>> 	* decl.cc (gfc_match_gcc_attributes): ... here. Add the attribute to
>>> 	the tree_list for the middle end.
>>>    
>> I prefer to not do any middle-end stuff at parsing time, so I would
>> rather not do this change.
>> Not OK.
> 
> Ok, that means we should filter-out those bits that we don't want to
> write to the module (right?). We've plenty of bits left, more than Dave
> Love would want to have added, i hope, so that should not be much of a
> concern.
> 
I didn't think of modules.  Yes, that means we have to store (in memory) 
the attribute we have parsed, and we can filter-out the attributes at 
the time the attributes are written to the module.  I don't think it is 
strictly necessary (for flatten, at least) though.

> What that table really wants to say is whether or not this attribute
> should be passed to the ME. Would it be acceptable to remove these
> duplicate strings and just have a bool/char/int that is true if it
> should be lowered (in trans-decl, as before)? But now i admit it's just
> bikeshedding and we can as well leave it alone for now.. It was just a
> though.
> 
Yes, that would be acceptable.
  

Patch

diff --git a/gcc/fortran/decl.cc b/gcc/fortran/decl.cc
index 3a619dbdd34..d312d4812b6 100644
--- a/gcc/fortran/decl.cc
+++ b/gcc/fortran/decl.cc
@@ -11802,15 +11802,15 @@  gfc_match_gcc_attribute_args (bool require_string, bool allow_multiple)
 }
 
 const ext_attr_t ext_attr_list[] = {
-  { "dllimport",    EXT_ATTR_DLLIMPORT,    "dllimport" },
-  { "dllexport",    EXT_ATTR_DLLEXPORT,    "dllexport" },
-  { "cdecl",        EXT_ATTR_CDECL,        "cdecl"     },
-  { "stdcall",      EXT_ATTR_STDCALL,      "stdcall"   },
-  { "fastcall",     EXT_ATTR_FASTCALL,     "fastcall"  },
-  { "no_arg_check", EXT_ATTR_NO_ARG_CHECK, NULL        },
-  { "deprecated",   EXT_ATTR_DEPRECATED,   NULL	       },
-  { "target_clones",EXT_ATTR_TARGET_CLONES,NULL	       },
-  { NULL,           EXT_ATTR_LAST,         NULL        }
+  { "dllimport",    EXT_ATTR_DLLIMPORT     },
+  { "dllexport",    EXT_ATTR_DLLEXPORT     },
+  { "cdecl",        EXT_ATTR_CDECL         },
+  { "stdcall",      EXT_ATTR_STDCALL       },
+  { "fastcall",     EXT_ATTR_FASTCALL,     },
+  { "no_arg_check", EXT_ATTR_NO_ARG_CHECK  },
+  { "deprecated",   EXT_ATTR_DEPRECATED    },
+  { "target_clones",EXT_ATTR_TARGET_CLONES },
+  { NULL,           EXT_ATTR_LAST          }
 };
 
 /* Match a !GCC$ ATTRIBUTES statement of the form:
@@ -11854,6 +11854,20 @@  gfc_match_gcc_attributes (void)
 	  gfc_error ("Unknown attribute in !GCC$ ATTRIBUTES statement at %C");
 	  return MATCH_ERROR;
 	}
+
+      /* Check for errors.
+	 If everything is fine, add attributes the middle-end has to know about.
+       */
+      if (!gfc_add_ext_attribute (&attr, (ext_attr_id_t)id, &gfc_current_locus))
+	return MATCH_ERROR;
+      else if (id == EXT_ATTR_DLLIMPORT
+	       || id == EXT_ATTR_DLLEXPORT
+	       || id == EXT_ATTR_CDECL
+	       || id == EXT_ATTR_STDCALL
+	       || id == EXT_ATTR_FASTCALL)
+	attr.ext_attr_args
+	  = chainon (attr.ext_attr_args,
+		     build_tree_list (get_identifier (name), NULL_TREE));
       else if (id == EXT_ATTR_TARGET_CLONES)
 	{
 	  attr_args
@@ -11864,9 +11878,6 @@  gfc_match_gcc_attributes (void)
 			 build_tree_list (get_identifier (name), attr_args));
 	}
 
-      if (!gfc_add_ext_attribute (&attr, (ext_attr_id_t)id, &gfc_current_locus))
-	return MATCH_ERROR;
-
       gfc_gobble_whitespace ();
       ch = gfc_next_ascii_char ();
       if (ch == ':')
diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index ce0cb61e647..c4deec0d5b8 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -847,7 +847,6 @@  typedef struct
 {
   const char *name;
   unsigned id;
-  const char *middle_end_name;
 }
 ext_attr_t;
 
diff --git a/gcc/fortran/trans-decl.cc b/gcc/fortran/trans-decl.cc
index 24cbd4cda28..7d5d2bdbb37 100644
--- a/gcc/fortran/trans-decl.cc
+++ b/gcc/fortran/trans-decl.cc
@@ -1436,18 +1436,7 @@  gfc_add_assign_aux_vars (gfc_symbol * sym)
 static tree
 add_attributes_to_decl (symbol_attribute sym_attr, tree list)
 {
-  unsigned id;
-  tree attr;
-
-  for (id = 0; id < EXT_ATTR_NUM; id++)
-    if (sym_attr.ext_attr & (1 << id) && ext_attr_list[id].middle_end_name)
-      {
-	attr = build_tree_list (
-		 get_identifier (ext_attr_list[id].middle_end_name),
-				 NULL_TREE);
-	list = chainon (list, attr);
-      }
-  /* Add attribute args.  */
+  /* Add attributes and their arguments.  */
   if (sym_attr.ext_attr_args != NULL_TREE)
     list = chainon (list, sym_attr.ext_attr_args);