diff mbox

Fix "naked" attribute (PR target/44290)

Message ID 4C495CBB.7010709@codesourcery.com
State New
Headers show

Commit Message

Jie Zhang July 23, 2010, 9:11 a.m. UTC 
On 07/23/2010 05:59 AM, Mark Mitchell wrote:
> Jie Zhang wrote:
>
>> The "naked" attribute should imply "noinline" and "noclone". This patch
>> implements this for all the 5 targets which currently support the
>> "naked" attribute: ARM, AVR, MCORE, RX, SPU.
>
> I completely agree with the spirit of this patch.  "naked" functions are
> very weird things, and the compiler should be very cautious in doing
> things with them.  It shouldn't make copies of them (inline or clone)
> because we really have no way of knowing what's in there, including (for
> example) static data declared in assembly code.
>
> But, I think the implementation could be more architecture-independent.
>   I'm prepared to say that any architecture that supports the "naked"
> attribute *must* implement it with the semantics on the architectures
> you name above.  That means that the ${arch}_insert_attributes functions
> you have could be architecture-independent.  Just put that code into the
> architecture-independent attribute-processing function.
>
This is a good idea. The modified patch is attached. Is it OK now?


Regards,

Comments

Mark Mitchell July 23, 2010, 2:29 p.m. UTC | #1 
Jie Zhang wrote:

>> But, I think the implementation could be more architecture-independent.
>>   I'm prepared to say that any architecture that supports the "naked"
>> attribute *must* implement it with the semantics on the architectures
>> you name above.  That means that the ${arch}_insert_attributes functions
>> you have could be architecture-independent.  Just put that code into the
>> architecture-independent attribute-processing function.
>>
> This is a good idea. The modified patch is attached. Is it OK now?

This patch is OK.

I thought a little bit about whether the manual should document
explicitly that naked functions will not be inlined (or cloned).  I
don't really think that's necessary, but if other people do, please
respond to such a request.

Thanks,
Jie Zhang July 23, 2010, 2:49 p.m. UTC | #2 
On 07/23/2010 10:29 PM, Mark Mitchell wrote:
> Jie Zhang wrote:
>
>>> But, I think the implementation could be more architecture-independent.
>>>    I'm prepared to say that any architecture that supports the "naked"
>>> attribute *must* implement it with the semantics on the architectures
>>> you name above.  That means that the ${arch}_insert_attributes functions
>>> you have could be architecture-independent.  Just put that code into the
>>> architecture-independent attribute-processing function.
>>>
>> This is a good idea. The modified patch is attached. Is it OK now?
>
> This patch is OK.
>
Thanks. I have committed the patch on trunk.

> I thought a little bit about whether the manual should document
> explicitly that naked functions will not be inlined (or cloned).  I
> don't really think that's necessary, but if other people do, please
> respond to such a request.
>
OK.


Regards,
Jie Zhang July 27, 2010, 5:39 p.m. UTC | #3 
On 07/23/2010 05:11 PM, Jie Zhang wrote:
> 	* tree-sra.c (ipa_sra_preliminary_function_checks): Return
> 	false if ! tree_versionable_function_p.

I have just reverted this part of my patch. 
ipa_sra_preliminary_function_checks already contains checking of 
tree_versionable_function_p, but I didn't notice it. Sorry.
diff mbox

Patch


	PR target/44290
	* attribs.c (decl_attributes): Insert "noinline" and "noclone"
	if "naked".
	* tree-sra.c (ipa_sra_preliminary_function_checks): Return
	false if ! tree_versionable_function_p.

	testsuite/
	PR target/44290
	* gcc.dg/pr44290-1.c: New test.
	* gcc.dg/pr44290-2.c: New test.


Index: attribs.c
===================================================================
--- attribs.c	(revision 162396)
+++ attribs.c	(working copy)
@@ -276,6 +276,19 @@  decl_attributes (tree *node, tree attrib
 	TREE_VALUE (cur_attr) = chainon (opts, TREE_VALUE (cur_attr));
     }
 
+  /* A "naked" function attribute implies "noinline" and "noclone" for
+     those targets that support it.  */
+  if (TREE_CODE (*node) == FUNCTION_DECL
+      && lookup_attribute_spec (get_identifier ("naked"))
+      && lookup_attribute ("naked", attributes) != NULL)
+    {
+      if (lookup_attribute ("noinline", attributes) == NULL)
+	attributes = tree_cons (get_identifier ("noinline"), NULL, attributes);
+
+      if (lookup_attribute ("noclone", attributes) == NULL)
+	attributes = tree_cons (get_identifier ("noclone"),  NULL, attributes);
+    }
+
   targetm.insert_attributes (*node, &attributes);
 
   for (a = attributes; a; a = TREE_CHAIN (a))
Index: testsuite/gcc.dg/pr44290-1.c
===================================================================
--- testsuite/gcc.dg/pr44290-1.c	(revision 0)
+++ testsuite/gcc.dg/pr44290-1.c	(revision 0)
@@ -0,0 +1,18 @@ 
+/* { dg-do compile { target arm*-*-* avr-*-* mcore-*-* rx-*-* spu-*-* } } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+static void __attribute__((naked))
+foo(void *from, void *to)
+{
+  asm volatile("dummy"::"r"(from), "r"(to));
+}
+
+unsigned int fie[2];
+
+void fum(void *to)
+{
+  foo(fie, to);
+}
+
+/* { dg-final { scan-tree-dump "foo \\\(void \\\* from, void \\\* to\\\)" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
Index: testsuite/gcc.dg/pr44290-2.c
===================================================================
--- testsuite/gcc.dg/pr44290-2.c	(revision 0)
+++ testsuite/gcc.dg/pr44290-2.c	(revision 0)
@@ -0,0 +1,24 @@ 
+/* { dg-do compile { target arm*-*-* avr-*-* mcore-*-* rx-*-* spu-*-* } } */
+/* { dg-options "-O2 -fdump-tree-optimized" } */
+
+static unsigned long __attribute__((naked))
+foo (unsigned long base)
+{
+  asm volatile ("dummy");
+}
+unsigned long
+bar (void)
+{
+  static int start, set;
+
+  if (!set)
+    {
+      set = 1;
+      start = foo (0);
+    }
+
+  return foo (start);
+}
+
+/* { dg-final { scan-tree-dump "foo \\\(long unsigned int base\\\)" "optimized" } } */
+/* { dg-final { cleanup-tree-dump "optimized" } } */
Index: tree-sra.c
===================================================================
--- tree-sra.c	(revision 162396)
+++ tree-sra.c	(working copy)
@@ -4285,6 +4285,13 @@  modify_function (struct cgraph_node *nod
 static bool
 ipa_sra_preliminary_function_checks (struct cgraph_node *node)
 {
+  if (!tree_versionable_function_p (current_function_decl))
+    {
+      if (dump_file)
+	fprintf (dump_file, "Function isn't allowed to be versioned.\n");
+      return false;
+    }
+
   if (!cgraph_node_can_be_local_p (node))
     {
       if (dump_file)