DECL_ASSEMBLER_NAME and friends

Message ID 235e8ca5-65d4-ee29-386c-c4594ed35ffd@acm.org
State New
Headers show
Series
  • DECL_ASSEMBLER_NAME and friends
Related show

Commit Message

Nathan Sidwell Oct. 10, 2017, 1:03 p.m.
I have a patch cleaning up a bit of the C++ FE, which will involve 
hashing via DECL_ASSEMBLER_NAME.  however, all the items in that hash 
are known to have it set, so its mapping to a function call is undesirable.

This patch adds DECL_ASSEMBLER_NAME_RAW, which gets at the field 
directly.  I can then use that for my hash table.  The cleanup to use 
that is fairly trivial.

However, I also looked more deeply at DECL_ASSEMBLER_NAME_SET_P. It does 
more than the name suggests -- namely checking HAS_DECL_ASSEMBLER_NAME_P 
too.  There are about 72 uses of DECL_ASSEMBLER_NAME_SET_P and 
investigation showed only about 4 applying it to decls that are not 
HAS_DECL_ASSEMBLER_NAME_P.  So, I remove the HAS_DECL_ASSEMBLER_NAME_P 
from DECL_ASSEMBLER_NAME_SET_P and explicitly check it at the 4 
locations that need it.

In doing this I noticed a couple of items:

1) ipa-utils.h (type_in_anonymous_namespace_p) is applying 
HAS_DECL_ASSEMBLER_NAME_P to a type.  That's clearly wrong, It looks 
like a thinko for TYPE_NAME (t).  Making that change seems fine.

2) tree-ssa-structalias.c (alias_get_name) seemed overly complicated, so 
I reimplemented it.

I checked the target-specific code, and there are only two uses of 
DECL_ASSEMBLER_NAME_SET_P.  Both look safe to me (the avr one seems 
superfluous, and DECL_ASSEMBLER_NAME would be fine there.

booted on x86_64-linux

ok?

nathan

Comments

Richard Biener Oct. 11, 2017, 12:28 p.m. | #1
On Tue, Oct 10, 2017 at 3:03 PM, Nathan Sidwell <nathan@acm.org> wrote:
> I have a patch cleaning up a bit of the C++ FE, which will involve hashing
> via DECL_ASSEMBLER_NAME.  however, all the items in that hash are known to
> have it set, so its mapping to a function call is undesirable.
>
> This patch adds DECL_ASSEMBLER_NAME_RAW, which gets at the field directly.
> I can then use that for my hash table.  The cleanup to use that is fairly
> trivial.
>
> However, I also looked more deeply at DECL_ASSEMBLER_NAME_SET_P. It does
> more than the name suggests -- namely checking HAS_DECL_ASSEMBLER_NAME_P
> too.  There are about 72 uses of DECL_ASSEMBLER_NAME_SET_P and investigation
> showed only about 4 applying it to decls that are not
> HAS_DECL_ASSEMBLER_NAME_P.  So, I remove the HAS_DECL_ASSEMBLER_NAME_P from
> DECL_ASSEMBLER_NAME_SET_P and explicitly check it at the 4 locations that
> need it.
>
> In doing this I noticed a couple of items:
>
> 1) ipa-utils.h (type_in_anonymous_namespace_p) is applying
> HAS_DECL_ASSEMBLER_NAME_P to a type.  That's clearly wrong, It looks like a
> thinko for TYPE_NAME (t).  Making that change seems fine.
>
> 2) tree-ssa-structalias.c (alias_get_name) seemed overly complicated, so I
> reimplemented it.
>
> I checked the target-specific code, and there are only two uses of
> DECL_ASSEMBLER_NAME_SET_P.  Both look safe to me (the avr one seems
> superfluous, and DECL_ASSEMBLER_NAME would be fine there.
>
> booted on x86_64-linux
>
> ok?

Ok.

Thanks,
Richard.

> nathan
> --
> Nathan Sidwell
Jan Hubicka Oct. 11, 2017, 1:53 p.m. | #2
> On Tue, Oct 10, 2017 at 3:03 PM, Nathan Sidwell <nathan@acm.org> wrote:
> > I have a patch cleaning up a bit of the C++ FE, which will involve hashing
> > via DECL_ASSEMBLER_NAME.  however, all the items in that hash are known to
> > have it set, so its mapping to a function call is undesirable.
> >
> > This patch adds DECL_ASSEMBLER_NAME_RAW, which gets at the field directly.
> > I can then use that for my hash table.  The cleanup to use that is fairly
> > trivial.
> >
> > However, I also looked more deeply at DECL_ASSEMBLER_NAME_SET_P. It does
> > more than the name suggests -- namely checking HAS_DECL_ASSEMBLER_NAME_P
> > too.  There are about 72 uses of DECL_ASSEMBLER_NAME_SET_P and investigation
> > showed only about 4 applying it to decls that are not
> > HAS_DECL_ASSEMBLER_NAME_P.  So, I remove the HAS_DECL_ASSEMBLER_NAME_P from
> > DECL_ASSEMBLER_NAME_SET_P and explicitly check it at the 4 locations that
> > need it.
> >
> > In doing this I noticed a couple of items:
> >
> > 1) ipa-utils.h (type_in_anonymous_namespace_p) is applying
> > HAS_DECL_ASSEMBLER_NAME_P to a type.  That's clearly wrong, It looks like a
> > thinko for TYPE_NAME (t).  Making that change seems fine.

Yep, it is a thinko. Thanks for spotting it!

Honza
Nathan Sidwell Oct. 11, 2017, 3:28 p.m. | #3
On 10/11/2017 08:28 AM, Richard Biener wrote:
> On Tue, Oct 10, 2017 at 3:03 PM, Nathan Sidwell <nathan@acm.org> wrote:
>> I have a patch cleaning up a bit of the C++ FE, which will involve hashing
>> via DECL_ASSEMBLER_NAME.  however, all the items in that hash are known to
>> have it set, so its mapping to a function call is undesirable.

> Ok.

I broke the patch into two.  This is the part introducing 
DECL_ASSEMBLER_NAME_RAW.

nathan
Nathan Sidwell Oct. 11, 2017, 4:33 p.m. | #4
On 10/11/2017 11:28 AM, Nathan Sidwell wrote:
> On 10/11/2017 08:28 AM, Richard Biener wrote:
>> On Tue, Oct 10, 2017 at 3:03 PM, Nathan Sidwell <nathan@acm.org> wrote:
>>> I have a patch cleaning up a bit of the C++ FE, which will involve 
>>> hashing
>>> via DECL_ASSEMBLER_NAME.  however, all the items in that hash are 
>>> known to
>>> have it set, so its mapping to a function call is undesirable.
> 
>> Ok.
> 
> I broke the patch into two.  This is the part introducing 
> DECL_ASSEMBLER_NAME_RAW.

And this is the bit separating HAS_DECL_ASSEMBLER_NAME_P from 
DECL_ASSEMBLER_NAME_SET_P.

applying to trunk.

nathan

Patch

2017-10-10  Nathan Sidwell  <nathan@acm.org>

	* tree.h (DECL_ASSEMBLER_NAME_RAW): New.
	(SET_DECL_ASSEMBLER_NAME): Use it.
	(DECL_ASSEMBLER_NAME_SET_P): Likewise.  Don't check
	HAS_DECL_ASSEMBLER_NAME_P.
	* tree.c (decl_assembler_name): Use DECL_ASSEMBLER_NAME_RAW.
	* gimple-expr.c (gimple_decl_printable_name: Check
	HAS_DECL_ASSEMBLER_NAME_P too.
	* ipa-utils.h (type_in_anonymous_namespace_p): Check
	DECL_ASSEMBLER_NAME_SET_P of TYPE_NAME.
	(odr_type_p): No need to assert TYPE_NAME is a TYPE_DECL.
	* passes.c (rest_of_decl_compilation): Check
	HAS_DECL_ASSEMBLER_NAME_P too.
	* recog.c (verify_changes): Likewise.
	* tree-pretty-print.c (dump_decl_name): Likewise.
	* tree-ssa-structalias.c (alias_get_name): Likewise.  Reimplement.

	c/
	* c-decl.c (grokdeclarator): Check HAS_DECL_ASSEMBLER_NAME_P too.

	lto/
	* lto.c (mentions_vars_p_decl_with_vis): Use
	DECL_ASSEMBLER_NAME_RAW.
	(lto_fixup_prevailing_decls): Likewise.
	
Index: c/c-decl.c
===================================================================
--- c/c-decl.c	(revision 253502)
+++ c/c-decl.c	(working copy)
@@ -7011,7 +7011,8 @@  grokdeclarator (const struct c_declarato
 
   /* This is the earliest point at which we might know the assembler
      name of a variable.  Thus, if it's known before this, die horribly.  */
-    gcc_assert (!DECL_ASSEMBLER_NAME_SET_P (decl));
+    gcc_assert (!HAS_DECL_ASSEMBLER_NAME_P (decl)
+		|| !DECL_ASSEMBLER_NAME_SET_P (decl));
 
     if (warn_cxx_compat
 	&& VAR_P (decl)
Index: gimple-expr.c
===================================================================
--- gimple-expr.c	(revision 253502)
+++ gimple-expr.c	(working copy)
@@ -337,9 +337,8 @@  gimple_decl_printable_name (tree decl, i
   if (!DECL_NAME (decl))
     return NULL;
 
-  if (DECL_ASSEMBLER_NAME_SET_P (decl))
+  if (HAS_DECL_ASSEMBLER_NAME_P (decl) && DECL_ASSEMBLER_NAME_SET_P (decl))
     {
-      const char *str, *mangled_str;
       int dmgl_opts = DMGL_NO_OPTS;
 
       if (verbosity >= 2)
@@ -352,9 +351,10 @@  gimple_decl_printable_name (tree decl, i
 	    dmgl_opts |= DMGL_PARAMS;
 	}
 
-      mangled_str = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
-      str = cplus_demangle_v3 (mangled_str, dmgl_opts);
-      return (str) ? str : mangled_str;
+      const char *mangled_str
+	= IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME_RAW (decl));
+      const char *str = cplus_demangle_v3 (mangled_str, dmgl_opts);
+      return str ? str : mangled_str;
     }
 
   return IDENTIFIER_POINTER (DECL_NAME (decl));
Index: ipa-utils.h
===================================================================
--- ipa-utils.h	(revision 253502)
+++ ipa-utils.h	(working copy)
@@ -217,11 +217,11 @@  type_in_anonymous_namespace_p (const_tre
     {
       /* C++ FE uses magic <anon> as assembler names of anonymous types.
  	 verify that this match with type_in_anonymous_namespace_p.  */
-      gcc_checking_assert (!in_lto_p || !DECL_ASSEMBLER_NAME_SET_P (t)
-			   || !strcmp
-				 ("<anon>",
-				  IDENTIFIER_POINTER
-				     (DECL_ASSEMBLER_NAME (TYPE_NAME (t)))));
+      gcc_checking_assert (!in_lto_p
+			   || !DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t))
+			   || !strcmp ("<anon>",
+				       IDENTIFIER_POINTER
+				       (DECL_ASSEMBLER_NAME (TYPE_NAME (t)))));
       return true;
     }
   return false;
@@ -245,14 +245,13 @@  odr_type_p (const_tree t)
   if (type_in_anonymous_namespace_p (t))
     return true;
 
-  if (TYPE_NAME (t) && TREE_CODE (TYPE_NAME (t)) == TYPE_DECL
-      && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))
+  if (TYPE_NAME (t) && DECL_ASSEMBLER_NAME_SET_P (TYPE_NAME (t)))
     {
       /* C++ FE uses magic <anon> as assembler names of anonymous types.
  	 verify that this match with type_in_anonymous_namespace_p.  */
       gcc_checking_assert (strcmp ("<anon>",
-				      IDENTIFIER_POINTER
-					(DECL_ASSEMBLER_NAME (TYPE_NAME (t)))));
+				   IDENTIFIER_POINTER
+				   (DECL_ASSEMBLER_NAME (TYPE_NAME (t)))));
       return true;
     }
   return false;
Index: lto/lto.c
===================================================================
--- lto/lto.c	(revision 253502)
+++ lto/lto.c	(working copy)
@@ -591,7 +591,7 @@  mentions_vars_p_decl_with_vis (tree t)
     return true;
 
   /* Accessor macro has side-effects, use field-name here. */
-  CHECK_NO_VAR (t->decl_with_vis.assembler_name);
+  CHECK_NO_VAR (DECL_ASSEMBLER_NAME_RAW (t));
   return false;
 }
 
@@ -2557,7 +2557,7 @@  lto_fixup_prevailing_decls (tree t)
 	}
       if (CODE_CONTAINS_STRUCT (code, TS_DECL_WITH_VIS))
 	{
-	  LTO_NO_PREVAIL (t->decl_with_vis.assembler_name);
+	  LTO_NO_PREVAIL (DECL_ASSEMBLER_NAME_RAW (t));
 	}
       if (CODE_CONTAINS_STRUCT (code, TS_DECL_NON_COMMON))
 	{
Index: passes.c
===================================================================
--- passes.c	(revision 253502)
+++ passes.c	(working copy)
@@ -197,7 +197,9 @@  rest_of_decl_compilation (tree decl,
 
   /* Can't defer this, because it needs to happen before any
      later function definitions are processed.  */
-  if (DECL_ASSEMBLER_NAME_SET_P (decl) && DECL_REGISTER (decl))
+  if (HAS_DECL_ASSEMBLER_NAME_P (decl)
+      && DECL_ASSEMBLER_NAME_SET_P (decl)
+      && DECL_REGISTER (decl))
     make_decl_rtl (decl);
 
   /* Forward declarations for nested functions are not "external",
Index: recog.c
===================================================================
--- recog.c	(revision 253502)
+++ recog.c	(working copy)
@@ -408,6 +408,7 @@  verify_changes (int num)
 	       && REG_P (changes[i].old)
 	       && asm_noperands (PATTERN (object)) > 0
 	       && REG_EXPR (changes[i].old) != NULL_TREE
+	       && HAS_DECL_ASSEMBLER_NAME_P (REG_EXPR (changes[i].old))
 	       && DECL_ASSEMBLER_NAME_SET_P (REG_EXPR (changes[i].old))
 	       && DECL_REGISTER (REG_EXPR (changes[i].old)))
 	{
Index: tree-pretty-print.c
===================================================================
--- tree-pretty-print.c	(revision 253502)
+++ tree-pretty-print.c	(working copy)
@@ -249,8 +249,10 @@  dump_decl_name (pretty_printer *pp, tree
 {
   if (DECL_NAME (node))
     {
-      if ((flags & TDF_ASMNAME) && DECL_ASSEMBLER_NAME_SET_P (node))
-	pp_tree_identifier (pp, DECL_ASSEMBLER_NAME (node));
+      if ((flags & TDF_ASMNAME)
+	  && HAS_DECL_ASSEMBLER_NAME_P (node)
+	  && DECL_ASSEMBLER_NAME_SET_P (node))
+	pp_tree_identifier (pp, DECL_ASSEMBLER_NAME_RAW (node));
       /* For DECL_NAMELESS names look for embedded uids in the
 	 names and sanitize them for TDF_NOUID.  */
       else if ((flags & TDF_NOUID) && DECL_NAMELESS (node))
Index: tree-ssa-structalias.c
===================================================================
--- tree-ssa-structalias.c	(revision 253502)
+++ tree-ssa-structalias.c	(working copy)
@@ -2849,41 +2849,33 @@  lookup_vi_for_tree (tree t)
 static const char *
 alias_get_name (tree decl)
 {
-  const char *res = NULL;
-  char *temp;
-
-  if (!dump_file)
-    return "NULL";
-
-  if (TREE_CODE (decl) == SSA_NAME)
+  const char *res = "NULL";
+  if (dump_file)
     {
-      res = get_name (decl);
-      if (res)
-	temp = xasprintf ("%s_%u", res, SSA_NAME_VERSION (decl));
-      else
-	temp = xasprintf ("_%u", SSA_NAME_VERSION (decl));
-      res = ggc_strdup (temp);
-      free (temp);
-    }
-  else if (DECL_P (decl))
-    {
-      if (DECL_ASSEMBLER_NAME_SET_P (decl))
-	res = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME (decl));
-      else
+      char *temp = NULL;
+      if (TREE_CODE (decl) == SSA_NAME)
+	{
+	  res = get_name (decl);
+	  temp = xasprintf ("%s_%u", res ? res : "", SSA_NAME_VERSION (decl));
+	}
+      else if (HAS_DECL_ASSEMBLER_NAME_P (decl)
+	       && DECL_ASSEMBLER_NAME_SET_P (decl))
+	res = IDENTIFIER_POINTER (DECL_ASSEMBLER_NAME_RAW (decl));
+      else if (DECL_P (decl))
 	{
 	  res = get_name (decl);
 	  if (!res)
-	    {
-	      temp = xasprintf ("D.%u", DECL_UID (decl));
-	      res = ggc_strdup (temp);
-	      free (temp);
-	    }
+	    temp = xasprintf ("D.%u", DECL_UID (decl));
+	}
+
+      if (temp)
+	{
+	  res = ggc_strdup (temp);
+	  free (temp);
 	}
     }
-  if (res != NULL)
-    return res;
 
-  return "NULL";
+  return res;
 }
 
 /* Find the variable id for tree T in the map.
Index: tree.c
===================================================================
--- tree.c	(revision 253502)
+++ tree.c	(working copy)
@@ -671,7 +671,7 @@  decl_assembler_name (tree decl)
 {
   if (!DECL_ASSEMBLER_NAME_SET_P (decl))
     lang_hooks.set_decl_assembler_name (decl);
-  return DECL_WITH_VIS_CHECK (decl)->decl_with_vis.assembler_name;
+  return DECL_ASSEMBLER_NAME_RAW (decl);
 }
 
 /* When the target supports COMDAT groups, this indicates which group the
Index: tree.h
===================================================================
--- tree.h	(revision 253502)
+++ tree.h	(working copy)
@@ -2722,6 +2722,10 @@  extern void decl_value_expr_insert (tree
    LTO compilation and C++.  */
 #define DECL_ASSEMBLER_NAME(NODE) decl_assembler_name (NODE)
 
+/* Raw accessor for DECL_ASSEMBLE_NAME.  */
+#define DECL_ASSEMBLER_NAME_RAW(NODE) \
+  (DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.assembler_name)
+
 /* Return true if NODE is a NODE that can contain a DECL_ASSEMBLER_NAME.
    This is true of all DECL nodes except FIELD_DECL.  */
 #define HAS_DECL_ASSEMBLER_NAME_P(NODE) \
@@ -2731,12 +2735,11 @@  extern void decl_value_expr_insert (tree
    the NODE might still have a DECL_ASSEMBLER_NAME -- it just hasn't been set
    yet.  */
 #define DECL_ASSEMBLER_NAME_SET_P(NODE) \
-  (HAS_DECL_ASSEMBLER_NAME_P (NODE) \
-   && DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.assembler_name != NULL_TREE)
+  (DECL_ASSEMBLER_NAME_RAW (NODE) != NULL_TREE)
 
 /* Set the DECL_ASSEMBLER_NAME for NODE to NAME.  */
 #define SET_DECL_ASSEMBLER_NAME(NODE, NAME) \
-  (DECL_WITH_VIS_CHECK (NODE)->decl_with_vis.assembler_name = (NAME))
+  (DECL_ASSEMBLER_NAME_RAW (NODE) = (NAME))
 
 /* Copy the DECL_ASSEMBLER_NAME from DECL1 to DECL2.  Note that if DECL1's
    DECL_ASSEMBLER_NAME has not yet been set, using this macro will not cause