Re: [patch] 19/n: trans-mem: compiler tree/gimple stuff

[Aldy Hernandez <aldyh@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2401 bytes --]

[rth, see below]

>> Index: gcc/attribs.c
>> ===================================================================
>> --- gcc/attribs.c       (.../trunk)     (revision 180744)
>> +++ gcc/attribs.c       (.../branches/transactional-memory)     (revision
>> 180773)
>> @@ -166,7 +166,8 @@ init_attributes (void)
>>           gcc_assert (strcmp (attribute_tables[i][j].name,
>>                               attribute_tables[i][k].name));
>>      }
>> -  /* Check that no name occurs in more than one table.  */
>> +  /* Check that no name occurs in more than one table.  Names that
>> +     begin with '*' are exempt, and may be overridden.  */
>>    for (i = 0; i<  ARRAY_SIZE (attribute_tables); i++)
>>      {
>>        size_t j, k, l;
>> @@ -174,8 +175,9 @@ init_attributes (void)
>>        for (j = i + 1; j<  ARRAY_SIZE (attribute_tables); j++)
>>         for (k = 0; attribute_tables[i][k].name != NULL; k++)
>>           for (l = 0; attribute_tables[j][l].name != NULL; l++)
>> -           gcc_assert (strcmp (attribute_tables[i][k].name,
>> -                               attribute_tables[j][l].name));
>> +           gcc_assert (attribute_tables[i][k].name[0] == '*'
>> +                       || strcmp (attribute_tables[i][k].name,
>> +                                  attribute_tables[j][l].name));
>>      }
>>   #endif
>>
>> @@ -207,7 +209,7 @@ register_attribute (const struct attribu
>>    slot = htab_find_slot_with_hash (attribute_hash,&str,
>>                                    substring_hash (str.str, str.length),
>>                                    INSERT);
>> -  gcc_assert (!*slot);
>> +  gcc_assert (!*slot || attr->name[0] == '*');
>>    *slot = (void *) CONST_CAST (struct attribute_spec *, attr);
>>   }
>
> The above changes seem to belong to a different changeset and look
> strange.  Why would attributes ever appear in two different tables?

I couldn't find a corresponding gcc-patches message for this patch, but 
I was able to hunt down full the patch, which I am attaching for discussion.

This seems to be a change required for allowing '*' to override 
builtins, so it is indeed part of the branch.  Perhaps with the full 
context it is easier to review.

I will defer to rth to answer any questions on the original motivation.

Richi, do you have any particular issue with the attribs.c change?  Does 
this context resolve any questions you may have had?

Aldy

[-- Attachment #2: x --]
[-- Type: text/plain, Size: 12093 bytes --]

Index: ChangeLog.tm
===================================================================
--- ChangeLog.tm	(revision 149303)
+++ ChangeLog.tm	(revision 149304)
@@ -1,3 +1,17 @@
+2009-07-06  Richard Henderson  <rth@redhat.com>
+
+	* attribs.c (init_attributes): Allow '*' prefix for overrides.
+	(register_attribute): Likewise.
+	* builtin-attrs.def (ATTR_TM_REGPARM): New.
+	(ATTR_TM_NOTHROW_LIST, ATTR_TM_NORETURN_NOTHROW_LIST,
+	ATTR_TM_NOTHROW_NONNULL, ATTR_TM_CONST_NOTHROW_LIST,
+	ATTR_TM_PURE_NOTHROW_LIST): New.
+	* c-common.c (ignore_attribute): New.
+	(c_common_attribute_table): Add "*tm regparm".
+
+	* config/i386/i386.c (ix86_handle_tm_regparm_attribute): New.
+	(ix86_attribute_table): Add "*tm regparm".
+
 2009-07-02  Richard Henderson  <rth@redhat.com>
 
 	* c-typeck.c (c_finish_tm_atomic): Use build_stmt.
Index: attribs.c
===================================================================
--- attribs.c	(revision 149303)
+++ attribs.c	(revision 149304)
@@ -166,7 +166,8 @@ init_attributes (void)
 	  gcc_assert (strcmp (attribute_tables[i][j].name,
 			      attribute_tables[i][k].name));
     }
-  /* Check that no name occurs in more than one table.  */
+  /* Check that no name occurs in more than one table.  Names that
+     begin with '*' are exempt, and may be overridden.  */
   for (i = 0; i < ARRAY_SIZE (attribute_tables); i++)
     {
       size_t j, k, l;
@@ -174,8 +175,9 @@ init_attributes (void)
       for (j = i + 1; j < ARRAY_SIZE (attribute_tables); j++)
 	for (k = 0; attribute_tables[i][k].name != NULL; k++)
 	  for (l = 0; attribute_tables[j][l].name != NULL; l++)
-	    gcc_assert (strcmp (attribute_tables[i][k].name,
-				attribute_tables[j][l].name));
+	    gcc_assert (attribute_tables[i][k].name[0] == '*'
+			|| strcmp (attribute_tables[i][k].name,
+				   attribute_tables[j][l].name));
     }
 #endif
 
@@ -202,7 +204,7 @@ register_attribute (const struct attribu
   slot = htab_find_slot_with_hash (attribute_hash, &str,
 				   substring_hash (str.str, str.length),
 				   INSERT);
-  gcc_assert (!*slot);
+  gcc_assert (!*slot || attr->name[0] == '*');
   *slot = (void *) CONST_CAST (struct attribute_spec *, attr);
 }
 
Index: builtin-attrs.def
===================================================================
--- builtin-attrs.def	(revision 149303)
+++ builtin-attrs.def	(revision 149304)
@@ -94,6 +94,7 @@ DEF_ATTR_IDENT (ATTR_SENTINEL, "sentinel
 DEF_ATTR_IDENT (ATTR_STRFMON, "strfmon")
 DEF_ATTR_IDENT (ATTR_STRFTIME, "strftime")
 DEF_ATTR_IDENT (ATTR_TYPEGENERIC, "type generic")
+DEF_ATTR_IDENT (ATTR_TM_REGPARM, "*tm regparm")
 
 DEF_ATTR_TREE_LIST (ATTR_NOVOPS_LIST, ATTR_NOVOPS, ATTR_NULL, ATTR_NULL)
 
@@ -192,6 +193,19 @@ DEF_FORMAT_ATTRIBUTE_NOTHROW(STRFMON,3,3
 #undef DEF_FORMAT_ATTRIBUTE_NOTHROW
 #undef DEF_FORMAT_ATTRIBUTE_BOTH
 
+/* Transactional memory variants of the above.  */
+
+DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_LIST,
+		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_NOTHROW_LIST)
+DEF_ATTR_TREE_LIST (ATTR_TM_NORETURN_NOTHROW_LIST,
+		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_NORETURN_NOTHROW_LIST)
+DEF_ATTR_TREE_LIST (ATTR_TM_NOTHROW_NONNULL,
+		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_NOTHROW_NONNULL)
+DEF_ATTR_TREE_LIST (ATTR_TM_CONST_NOTHROW_LIST,
+		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_CONST_NOTHROW_LIST)
+DEF_ATTR_TREE_LIST (ATTR_TM_PURE_NOTHROW_LIST,
+		    ATTR_TM_REGPARM, ATTR_NULL, ATTR_PURE_NOTHROW_LIST)
+
 /* Construct a tree for a format_arg attribute.  */
 #define DEF_FORMAT_ARG_ATTRIBUTE(FA)					\
   DEF_ATTR_TREE_LIST (ATTR_FORMAT_ARG_##FA, ATTR_FORMAT_ARG,		\
Index: testsuite/gcc.dg/tm/indirect-1.c
===================================================================
--- testsuite/gcc.dg/tm/indirect-1.c	(revision 0)
+++ testsuite/gcc.dg/tm/indirect-1.c	(revision 149304)
@@ -0,0 +1,9 @@
+/* { dg-do compile } */
+/* { dg-options "-fgnu-tm" } */
+
+void foo(void (*fn)(void))
+{
+  __tm_atomic {
+    fn();
+  }
+}
Index: except.c
===================================================================
--- except.c	(revision 149303)
+++ except.c	(revision 149304)
@@ -2895,13 +2895,18 @@ remove_eh_handler_and_replace (struct eh
     }
 }
 
-/* Splice REGION from the region tree and replace it by the outer region
-   etc.  */
+/* Splice REGION from the region tree and replace it by an outer region.  */
 
 static void
 remove_eh_handler (struct eh_region_d *region)
 {
-  remove_eh_handler_and_replace (region, region->outer, true);
+  struct eh_region_d *outer;
+
+  for (outer = region->outer; outer; outer = outer->outer)
+    if (outer->type != ERT_TRANSACTION)
+      break;
+
+  remove_eh_handler_and_replace (region, outer, true);
 }
 
 /* Remove Eh region R that has turned out to have no code in its handler.  */
Index: c-common.c
===================================================================
--- c-common.c	(revision 149303)
+++ c-common.c	(revision 149304)
@@ -530,6 +530,7 @@ static tree handle_type_generic_attribut
 static tree handle_alloc_size_attribute (tree *, tree, tree, int, bool *);
 static tree handle_target_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 *);
 
 static void check_function_nonnull (tree, int, tree *);
 static void check_nonnull_arg (void *, tree, unsigned HOST_WIDE_INT);
@@ -830,6 +831,10 @@ const struct attribute_spec c_common_att
 			      handle_target_attribute },
   { "optimize",               1, -1, true, false, false,
 			      handle_optimize_attribute },
+  /* For internal use only.  The leading '*' both prevents its usage in
+     source code and signals that it may be overridden by machine tables.  */
+  { "*tm regparm",	      0, 0, false, true, true,
+                              ignore_attribute },
   { NULL,                     0, 0, false, false, false, NULL }
 };
 
@@ -7865,6 +7870,19 @@ handle_optimize_attribute (tree *node, t
 
   return NULL_TREE;
 }
+
+/* Ignore the given attribute.  Used when this attribute may be usefully
+   overridden by the target, but is not used generically.  */
+
+static tree
+ignore_attribute (tree *node, tree ARG_UNUSED (name), tree ARG_UNUSED (args),
+		  int ARG_UNUSED (flags), bool *no_add_attrs)
+{
+  *no_add_attrs = true;
+  return NULL_TREE;
+}
+
+
 \f
 /* Check for valid arguments being passed to a function.
    ATTRS is a list of attributes.  There are NARGS arguments in the array
Index: gtm-builtins.def
===================================================================
--- gtm-builtins.def	(revision 149303)
+++ gtm-builtins.def	(revision 149304)
@@ -1,49 +1,49 @@
 DEF_TM_BUILTIN (BUILT_IN_TM_START, "_ITM_beginTransaction",
-		BT_FN_UINT_UINT, ATTR_NOTHROW_LIST)
+		BT_FN_UINT_UINT, ATTR_TM_NOTHROW_LIST)
 
 DEF_TM_BUILTIN (BUILT_IN_TM_COMMIT, "_ITM_commitTransaction",
-		BT_FN_VOID, ATTR_NOTHROW_LIST)
+		BT_FN_VOID, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_ABORT, "_ITM_abortTransaction",
-		BT_FN_INT, ATTR_NORETURN_NOTHROW_LIST)
+		BT_FN_INT, ATTR_TM_NORETURN_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_IRREVOKABLE, "_ITM_changeTransactionMode",
-		BT_FN_INT, ATTR_NOTHROW_LIST)
+		BT_FN_INT, ATTR_TM_NOTHROW_LIST)
 
 DEF_TM_BUILTIN (BUILT_IN_TM_MEMCPY, "_ITM_memcpyRtWt",
-		BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_NOTHROW_NONNULL)
+		BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_TM_NOTHROW_NONNULL)
 DEF_TM_BUILTIN (BUILT_IN_TM_MEMMOVE, "_ITM_memmoveRtWt",
-		BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_NOTHROW_NONNULL)
+		BT_FN_PTR_PTR_CONST_PTR_SIZE, ATTR_TM_NOTHROW_NONNULL)
 
 DEF_TM_BUILTIN (BUILT_IN_TM_GETTMCLONE_IRR, "_ITM_getTMCloneOrIrrevokable",
-		BT_FN_PTR_PTR, ATTR_NOTHROW_NONNULL)
+		BT_FN_PTR_PTR, ATTR_TM_NOTHROW_NONNULL)
 DEF_TM_BUILTIN (BUILT_IN_TM_GETTMCLONE_SAFE, "_ITM_getTMCloneSafe",
-		BT_FN_PTR_PTR, ATTR_CONST_NOTHROW_LIST)
+		BT_FN_PTR_PTR, ATTR_TM_CONST_NOTHROW_LIST)
 
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_1, "_ITM_WU1",
-		BT_FN_VOID_VPTR_I1, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_I1, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_2, "_ITM_WU2",
-		BT_FN_VOID_VPTR_I2, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_I2, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_4, "_ITM_WU4",
-		BT_FN_VOID_VPTR_I4, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_I4, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_8, "_ITM_WU8",
-		BT_FN_VOID_VPTR_I8, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_I8, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_FLOAT, "_ITM_WF",
-		BT_FN_VOID_VPTR_FLOAT, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_FLOAT, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_DOUBLE, "_ITM_WD",
-		BT_FN_VOID_VPTR_DOUBLE, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_DOUBLE, ATTR_TM_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_STORE_LDOUBLE, "_ITM_WE",
-		BT_FN_VOID_VPTR_LDOUBLE, ATTR_NOTHROW_LIST)
+		BT_FN_VOID_VPTR_LDOUBLE, ATTR_TM_NOTHROW_LIST)
 
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_1, "_ITM_RU1",
-		BT_FN_I1_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_I1_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_2, "_ITM_RU2",
-		BT_FN_I2_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_I2_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_4, "_ITM_RU4",
-		BT_FN_I4_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_I4_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_8, "_ITM_RU8",
-		BT_FN_I8_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_I8_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_FLOAT, "_ITM_RF",
-		BT_FN_FLOAT_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_FLOAT_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_DOUBLE, "_ITM_RD",
-		BT_FN_DOUBLE_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_DOUBLE_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
 DEF_TM_BUILTIN (BUILT_IN_TM_LOAD_LDOUBLE, "_ITM_RE",
-		BT_FN_LDOUBLE_VPTR, ATTR_PURE_NOTHROW_LIST)
+		BT_FN_LDOUBLE_VPTR, ATTR_TM_PURE_NOTHROW_LIST)
Index: config/i386/i386.c
===================================================================
--- config/i386/i386.c	(revision 149303)
+++ config/i386/i386.c	(revision 149304)
@@ -4377,6 +4377,39 @@ ix86_handle_cconv_attribute (tree *node,
   return NULL_TREE;
 }
 
+/* The transactional memory builtins are implicitly regparm or fastcall
+   depending on the ABI.  Override the generic do-nothing attribute that
+   these builtins were declared with, and replace it with one of the two
+   attributes that we expect elsewhere.  */
+
+static tree
+ix86_handle_tm_regparm_attribute (tree *node, tree name, tree args,
+				  int flags ATTRIBUTE_UNUSED,
+				  bool *no_add_attrs)
+{
+  tree alt;
+
+  /* In no case do we want to add the placeholder attribute.  */
+  *no_add_attrs = true;
+
+  /* The 64-bit ABI is unchanged for transactional memory.  */
+  if (TARGET_64BIT)
+    return NULL_TREE;
+
+  /* ??? Is there a better way to validate 32-bit windows?  We have
+     cfun->machine->call_abi, but that seems to be set only for 64-bit.  */
+  if (CHECK_STACK_LIMIT > 0)
+    alt = tree_cons (get_identifier ("fastcall"), NULL, NULL);
+  else
+    {
+      alt = tree_cons (NULL, build_int_cst (NULL, 2), NULL);
+      alt = tree_cons (get_identifier ("regparm"), alt, NULL);
+    }
+  decl_attributes (node, alt, flags);
+
+  return NULL_TREE;
+}
+
 /* Return 0 if the attributes for two types are incompatible, 1 if they
    are compatible, and 2 if they are nearly compatible (which causes a
    warning to be generated).  */
@@ -30424,6 +30457,10 @@ static const struct attribute_spec ix86_
   /* Sseregparm attribute says we are using x86_64 calling conventions
      for FP arguments.  */
   { "sseregparm", 0, 0, false, true, true, ix86_handle_cconv_attribute },
+  /* The transactional memory builtins are implicitly regparm or fastcall
+     depending on the ABI.  Override the generic do-nothing attribute that
+     these builtins were declared with.  */
+  { "*tm regparm", 0, 0, false, true, true, ix86_handle_tm_regparm_attribute },
   /* force_align_arg_pointer says this function realigns the stack at entry.  */
   { (const char *)&ix86_force_align_arg_pointer_string, 0, 0,
     false, true,  true, ix86_handle_cconv_attribute },