diff mbox

ping: [PATCH, ARM] attribute target (thumb,arm) [0-6]

Message ID 5534BA3F.8060602@st.com
State New
Headers show

Commit Message

Christian Bruel April 20, 2015, 8:35 a.m. UTC 
Hello Ramana

>>
> 
> Can you respin this now that we are in stage1 again ?
> 
> Ramana
> 

Attached the rebased, rechecked set of patches. Original with comments
posted in

https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02455.html
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02458.html
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02460.html
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02461.html
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02463.html
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02467.html
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02468.html

many thanks,

Christian

Comments

Ramana Radhakrishnan April 30, 2015, 7:43 a.m. UTC | #1 
On Mon, Apr 20, 2015 at 9:35 AM, Christian Bruel <christian.bruel@st.com> wrote:
> Hello Ramana
>
>>>
>>
>> Can you respin this now that we are in stage1 again ?
>>
>> Ramana
>>
>
> Attached the rebased, rechecked set of patches. Original with comments
> posted in
>
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02455.html
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02458.html
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02460.html
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02461.html
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02463.html
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02467.html
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02468.html
>
> many thanks,
>
> Christian


A general note, please reply to each of the patches with a rebased
patch as a separate email. Further more all your patches appear to
have dos line endings so they don't seem to apply cleanly. Please
don't have spurious headers in your patch submission - it then makes
it hard to , please create it in a way that it is easily applied by
someone trying it out. It looks like p4 needs a respin as I got a
reject trying to apply the documentation patch to my tree while trying
to apply it.

I tried the following decoration on foo in gcc.target/arm/attr_arm.c


int __attribute__((target("arm, fpu=vfpv4")))
foo(int a)
{
  return a ? 1 : 5;
}


And the compiler accepts it just fine.

Given that with LTO we are now using target attributes to decide
inlining - I'm not convinced that the inline asm case goes away. In
fact it only makes things worse so I'm almost convinced to forbid
inlining from "arm" to "thumb" or vice-versa, which is a reversal of
my earlier position. I hadn't twigged that LTO would reuse this
infrastructure and it's probably simpler to prevent inlining in those
cases.

Thoughts ?

So in essence I'm still playing with this and would like to iterate
towards a quick solution.

Ramana
Christian Bruel April 30, 2015, 8:35 a.m. UTC | #2 
On 04/30/2015 09:43 AM, Ramana Radhakrishnan wrote:
> On Mon, Apr 20, 2015 at 9:35 AM, Christian Bruel <christian.bruel@st.com> wrote:
>> Hello Ramana
>>
>>>>
>>>
>>> Can you respin this now that we are in stage1 again ?
>>>
>>> Ramana
>>>
>>
>> Attached the rebased, rechecked set of patches. Original with comments
>> posted in
>>
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02455.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02458.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02460.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02461.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02463.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02467.html
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg02468.html
>>
>> many thanks,
>>
>> Christian
> 
> 
> A general note, please reply to each of the patches with a rebased
> patch as a separate email. Further more all your patches appear to
> have dos line endings so they don't seem to apply cleanly. Please
> don't have spurious headers in your patch submission - it then makes
> it hard to , please create it in a way that it is easily applied by
> someone trying it out. It looks like p4 needs a respin as I got a
> reject trying to apply the documentation patch to my tree while trying
> to apply it.
> 

OK, thanks for the suggestions and sorry for the p4 reject. The sources
are moving fast and I have hard times catching up with re-bases.

> I tried the following decoration on foo in gcc.target/arm/attr_arm.c
> 
> 
> int __attribute__((target("arm, fpu=vfpv4")))
> foo(int a)
> {
>   return a ? 1 : 5;
> }
> 
> 
> And the compiler accepts it just fine.

Indeed, it's a mistake for now. attributes other the arm/thumb ones
shall be rejected (eventually with a "not yet implemented" warning for
the fpu, error for the others.) until we extend it.

> 
> Given that with LTO we are now using target attributes to decide
> inlining - I'm not convinced that the inline asm case goes away. In
> fact it only makes things worse so I'm almost convinced to forbid
> inlining from "arm" to "thumb" or vice-versa, which is a reversal of
> my earlier position. I hadn't twigged that LTO would reuse this
> infrastructure and it's probably simpler to prevent inlining in those
> cases.

I can resurrect the inline check chunk. FYI, with a few small examples
arm/thumb attribute is correctly handled by LTO

> 
> Thoughts ?
> 
> So in essence I'm still playing with this and would like to iterate
> towards a quick solution.
> 

thanks, that would be good if we could land the arm/thumb attribute and
start the fpu extensions separately. (I'm currently playing with
fpu=neon but it will take time to have something solid).

Christian

> Ramana
>
Ramana Radhakrishnan April 30, 2015, 10 a.m. UTC | #3 
>>> Christian
>>
>>
>> A general note, please reply to each of the patches with a rebased
>> patch as a separate email. Further more all your patches appear to
>> have dos line endings so they don't seem to apply cleanly. Please
>> don't have spurious headers in your patch submission - it then makes
>> it hard to , please create it in a way that it is easily applied by
>> someone trying it out. It looks like p4 needs a respin as I got a
>> reject trying to apply the documentation patch to my tree while trying
>> to apply it.
>>
>
> OK, thanks for the suggestions and sorry for the p4 reject. The sources
> are moving fast and I have hard times catching up with re-bases.

I understand.

>
>> I tried the following decoration on foo in gcc.target/arm/attr_arm.c
>>
>>
>> int __attribute__((target("arm, fpu=vfpv4")))
>> foo(int a)
>> {
>>    return a ? 1 : 5;
>> }
>>
>>
>> And the compiler accepts it just fine.
>
> Indeed, it's a mistake for now. attributes other the arm/thumb ones
> shall be rejected (eventually with a "not yet implemented" warning for
> the fpu, error for the others.) until we extend it.

Yep - funnily enough if you remove "arm" and just use "fpu=vfpv4", I 
think you get an error.

>
>>
>> Given that with LTO we are now using target attributes to decide
>> inlining - I'm not convinced that the inline asm case goes away. In
>> fact it only makes things worse so I'm almost convinced to forbid
>> inlining from "arm" to "thumb" or vice-versa, which is a reversal of
>> my earlier position. I hadn't twigged that LTO would reuse this
>> infrastructure and it's probably simpler to prevent inlining in those
>> cases.
>
> I can resurrect the inline check chunk. FYI, with a few small examples
> arm/thumb attribute is correctly handled by LTO

Yes it would work with normal C code as it does normally - I'm worried 
about functions with inline asm. We've just increased the inlining scope 
with lto and that would mean things are a bit more painful ?


>
>>
>> Thoughts ?
>>
>> So in essence I'm still playing with this and would like to iterate
>> towards a quick solution.
>>
>
> thanks, that would be good if we could land the arm/thumb attribute and
> start the fpu extensions separately. (I'm currently playing with
> fpu=neon but it will take time to have something solid).

Absolutely - I'd rather spend the time first in polishing this up. 
Extending it for other options can be something you look at separately.

BTW I was pointed at a PR for this yesterday by a colleague - 
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59884

So, lets use that as the PR for this work.

regards
Ramana

>
> Christian
>
>> Ramana
>>
>
Christian Bruel May 20, 2015, 1:06 p.m. UTC | #4 
Hi Ramana,

>
> BTW I was pointed at a PR for this yesterday by a colleague -
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=59884
>
> So, lets use that as the PR for this work.
>
> regards
> Ramana
>
>

just to let you know that I've also found this old entry (2012):

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52144

I'm going to link all of those, and continue the review there

cheers

Christian
diff mbox

Patch

2014-09-23  Christian Bruel  <christian.bruel@st.com>

	* config/arm/arm.c (add_attribute, arm_insert_attributes): New functions
	(TARGET_INSERT_ATTRIBUTES): Define.
	(thumb_flipper): New var.
	* config/arm/arm.opt (-mflip-thumb): New switch.

diff -ruN '--exclude=.svn' gnu_trunk.devs5/gcc/gcc/config/arm/arm.c gnu_trunk.devs6/gcc/gcc/config/arm/arm.c
--- gnu_trunk.devs5/gcc/gcc/config/arm/arm.c	2015-02-05 09:21:44.449543733 +0100
+++ gnu_trunk.devs6/gcc/gcc/config/arm/arm.c	2015-02-05 09:21:56.821563455 +0100
@@ -99,6 +99,7 @@ 
 #include "tm-constrs.h"
 #include "rtl-iter.h"
 #include "sched-int.h"
+#include "tree.h"
 
 /* Forward definitions of types.  */
 typedef struct minipool_node    Mnode;
@@ -232,6 +233,7 @@ 
 
 static void arm_file_end (void);
 static void arm_file_start (void);
+static void arm_insert_attributes (tree, tree *);
 
 static void arm_setup_incoming_varargs (cumulative_args_t, machine_mode,
 					tree, int *, int);
@@ -390,6 +392,9 @@ 
 #undef  TARGET_ATTRIBUTE_TABLE
 #define TARGET_ATTRIBUTE_TABLE arm_attribute_table
 
+#undef  TARGET_INSERT_ATTRIBUTES
+#define TARGET_INSERT_ATTRIBUTES arm_insert_attributes
+
 #undef TARGET_ASM_FILE_START
 #define TARGET_ASM_FILE_START arm_file_start
 #undef TARGET_ASM_FILE_END
@@ -2717,6 +2722,10 @@ 
     }
 }
 
+/* True if -mflip-thumb should next add an attribute for the default
+   mode, false if it should next add an attribute for the opposite mode.  */
+static GTY(()) bool thumb_flipper;
+
 /* Options after initial target override.  */
 static GTY(()) tree init_optimize;
 
@@ -3282,6 +3291,9 @@ 
      options.  */
   target_option_default_node = target_option_current_node
     = build_target_option_node (&global_options);
+
+  /* Init initial mode for testing.  */
+  thumb_flipper = TARGET_THUMB;
 }
 
 static void
@@ -29624,6 +29636,52 @@ 
   return build_target_option_node (opts);
 }
 
+static void 
+add_attribute  (const char * mode, tree *attributes)
+{
+  size_t len = strlen (mode);
+  tree value = build_string (len, mode);
+
+  TREE_TYPE (value) = build_array_type (char_type_node,
+					build_index_type (size_int (len)));
+
+  *attributes = tree_cons (get_identifier ("target"),
+			   build_tree_list (NULL_TREE, value),
+			   *attributes);
+}
+
+/* For testing. Insert thumb or arm modes alternatively on functions.  */
+
+static void
+arm_insert_attributes (tree fndecl, tree * attributes)
+{
+  const char *mode;
+
+  if (! TARGET_FLIP_THUMB)
+    return;
+
+  if (TREE_CODE (fndecl) != FUNCTION_DECL || DECL_EXTERNAL(fndecl)
+      || DECL_BUILT_IN (fndecl) || DECL_ARTIFICIAL (fndecl))
+   return;
+
+  /* Nested definitions must inherit mode.  */
+  if (current_function_decl)
+   {
+     mode = TARGET_THUMB ? "thumb" : "arm";      
+     add_attribute (mode, attributes);
+     return;
+   }
+
+  /* If there is already a setting don't change it.  */
+  if (lookup_attribute ("target", *attributes) != NULL)
+    return;
+
+  mode = thumb_flipper ? "thumb" : "arm";
+  add_attribute (mode, attributes);
+
+  thumb_flipper = !thumb_flipper;
+}
+
 /* Hook to validate attribute((target("string"))).  */
 
 static bool
@@ -29635,13 +29693,15 @@ 
   tree cur_tree, new_optimize;
   gcc_assert ((fndecl != NULL_TREE) && (args != NULL_TREE));
 
+  tree old_optimize = build_optimization_node (&global_options);
+
   /* Get the optimization options of the current function.  */
   tree func_optimize = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl);
 
   /* If the function changed the optimization levels as well as setting target
      options, start with the optimizations specified.  */
   if (!func_optimize)
-    func_optimize = optimization_default_node;
+    func_optimize = old_optimize;
 
   /* Init func_options.  */
   memset (&func_options, 0, sizeof (func_options));
@@ -29659,14 +29719,17 @@ 
   cur_tree = arm_valid_target_attribute_tree (args, &func_options,
 					      &global_options_set);
 
-  if (cur_tree == NULL_TREE)
-    ret = false;
-
   new_optimize = build_optimization_node (&func_options);
 
-  DECL_FUNCTION_SPECIFIC_TARGET (fndecl) = cur_tree;
+  if (cur_tree == NULL_TREE)
+    ret = false;
+  else
+    {
+      DECL_FUNCTION_SPECIFIC_TARGET (fndecl) = cur_tree;
 
-  DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl) = new_optimize;
+      if (old_optimize != new_optimize)
+	DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl) = new_optimize;
+    }
 
   return ret;
 }
diff -ruN '--exclude=.svn' gnu_trunk.devs5/gcc/gcc/config/arm/arm.opt gnu_trunk.devs6/gcc/gcc/config/arm/arm.opt
--- gnu_trunk.devs5/gcc/gcc/config/arm/arm.opt	2015-01-22 13:56:45.255251418 +0100
+++ gnu_trunk.devs6/gcc/gcc/config/arm/arm.opt	2015-01-21 18:18:16.166984018 +0100
@@ -122,6 +122,10 @@ 
 EnumValue
 Enum(float_abi_type) String(hard) Value(ARM_FLOAT_ABI_HARD)
 
+mflip-thumb
+Target Report Var(TARGET_FLIP_THUMB)
+Switch ARM/Thumb modes on alternating functions for compiler testing
+
 mfp16-format=
 Target RejectNegative Joined Enum(arm_fp16_format_type) Var(arm_fp16_format) Init(ARM_FP16_FORMAT_NONE)
 Specify the __fp16 floating-point format