[PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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

This patch uses FUNCTION_BOUNDARY instead of DECL_ALIGN to check the max
align when optimizing for size in assemble_start_function.
This is necessary for ARM that can switch the max code alignment
directives between modes.

No regressions for ARM
Testing on-going for x86

Christian

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: align2.patch --]
[-- Type: text/x-patch; name="align2.patch", Size: 1503 bytes --]

2015-09-29  Christian Bruel  <christian.bruel@st.com>

	PR target/67745
	* gcc/varasm.c (assemble_start_function): Use current's function align.

Index: gcc/varasm.c
===================================================================
--- gcc/varasm.c	(revision 228229)
+++ gcc/varasm.c	(working copy)
@@ -1729,7 +1729,7 @@ assemble_start_function (tree decl, cons
   if (CONSTANT_POOL_BEFORE_FUNCTION)
     output_constant_pool (fnname, decl);
 
-  align = symtab_node::get (decl)->definition_alignment ();
+  align = FUNCTION_BOUNDARY;
 
   /* Make sure the not and cold text (code) sections are properly
      aligned.  This is necessary here in the case where the function
@@ -1774,12 +1774,15 @@ assemble_start_function (tree decl, cons
       ASM_OUTPUT_ALIGN (asm_out_file, align);
     }
 
+  /* align_functions_log cannot exceed current function's ABI when
+     optimizing for size  */
+  if (optimize_function_for_size_p (cfun))
+    align_functions_log = MIN (align_functions_log, align);
+
   /* Handle a user-specified function alignment.
      Note that we still need to align to DECL_ALIGN, as above,
      because ASM_OUTPUT_MAX_SKIP_ALIGN might not do any alignment at all.  */
-  if (! DECL_USER_ALIGN (decl)
-      && align_functions_log > align
-      && optimize_function_for_speed_p (cfun))
+  if (! DECL_USER_ALIGN (decl) && align_functions_log > align)
     {
 #ifdef ASM_OUTPUT_MAX_SKIP_ALIGN
       ASM_OUTPUT_MAX_SKIP_ALIGN (asm_out_file,

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

On 09/29/2015 03:24 PM, Christian Bruel wrote:
> This patch uses FUNCTION_BOUNDARY instead of DECL_ALIGN to check the max
> align when optimizing for size in assemble_start_function.
> This is necessary for ARM that can switch the max code alignment
> directives between modes.
> 
s/max/min

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Jeff Law]

On 09/29/2015 07:24 AM, Christian Bruel wrote:
> This patch uses FUNCTION_BOUNDARY instead of DECL_ALIGN to check the max
> align when optimizing for size in assemble_start_function.
> This is necessary for ARM that can switch the max code alignment
> directives between modes.
>
> No regressions for ARM
> Testing on-going for x86
>
> Christian
>
>
> align2.patch
>
>
> 2015-09-29  Christian Bruel<christian.bruel@st.com>
>
> 	PR target/67745
> 	* gcc/varasm.c (assemble_start_function): Use current's function align.
Does this override alignment information that might be attached to the 
DECL?   Does that, in effect, override any alignment information that 
the developer may have put on the decl?  If so, then it seems like a bad 
idea, even with -Os.

Am I missing something here?

jeff

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

On 09/30/2015 09:02 PM, Jeff Law wrote:
> On 09/29/2015 07:24 AM, Christian Bruel wrote:
>> This patch uses FUNCTION_BOUNDARY instead of DECL_ALIGN to check the max
>> align when optimizing for size in assemble_start_function.
>> This is necessary for ARM that can switch the max code alignment
>> directives between modes.
>>
>> No regressions for ARM
>> Testing on-going for x86
>>
>> Christian
>>
>>
>> align2.patch
>>
>>
>> 2015-09-29  Christian Bruel<christian.bruel@st.com>
>>
>> 	PR target/67745
>> 	* gcc/varasm.c (assemble_start_function): Use current's function align.
> Does this override alignment information that might be attached to the 
> DECL?  

Yes. Well, in the backend, for architectural (ABI) defined alignment,
FUNCTION_BOUNDARY is used instead of DECL_ALIGN. And DECL_ALIGN is set
with FUNCTION_BOUNDARY (make_node_stat) so they are accidentally
equivalent if FUNCTION_BOUNDARY does not get changed due to switchable
target.

 Does that, in effect, override any alignment information that
> the developer may have put on the decl?  If so, then it seems like a bad 
> idea, even with -Os.

No it doesn't affect user alignments, that are carried thru
align_functions_log, that prevails over the DECL_ALIGN.
I played with various configurations of -falign-functions=x,
__attribute__((aligned(x))), optimization levels for arm and x86 and was
happy with them.

> 
> Am I missing something here?

Your comment makes me think about the direction I've taken to replace
DECL_ALIGN by FUNCTION_BOUNDARY, and the problem might not be user align
vs DECL_ALIGN: I stepped upon Jan's patch for a fortran regression.

2015-03-05  Jan Hubicka  <hubicka@ucw.cz>

	PR ipa/65334
	* cgraph.h (symtab_node): Add definition_alignment,

My patch was tested only for languages=c/c++. I'm going to give it a try
with Fortran.
If this is not satisfactory I'll have to think about updating DECL_ALIGN
after the function's attribute are processed and keep using it in
assemble_start_function.

Come back later

thanks,

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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



> If this is not satisfactory I'll have to think about updating DECL_ALIGN
> after the function's attribute are processed and keep using it in
> assemble_start_function.
> 
> Come back later
> 

Indeed, I found that resetting DECL_ALIGN in the
arm_set_current_function hooks is much cleaner than my previous patch,
and, as you said, preserve the case where DECL_USER_ALIGN is 1.

First I thought it was weird to change a tree fndecl field in the
targetm.set_current_function (fndecl) hook as I haven't seen in any
other target and that only the restore_target_globals or
cl_target_option_restore functions would switch the states between
functions (like align_functions_log, target_flags) thanks to
target_reinit. But it seems to be the less intrusive one line fix I
found without touching the middle end parts. Other suggestions welcome
though.

I'm testing with the attached patch to replace the previous (2/2) one
and I'll discuss it next week with the arm maintainers after more testing.
sorry for the noise.

thanks


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: align2.patch --]
[-- Type: text/x-patch; name="align2.patch", Size: 547 bytes --]

diff '--exclude=.svn' -rup gnu_trunk.devs/gcc/gcc/config/arm/arm.c gnu_trunk.devs1/gcc/gcc/config/arm/arm.c
--- gnu_trunk.devs/gcc/gcc/config/arm/arm.c	2015-10-01 17:50:22.996545547 +0200
+++ gnu_trunk.devs1/gcc/gcc/config/arm/arm.c	2015-10-01 17:56:01.213272650 +0200
@@ -29808,6 +29808,9 @@ arm_set_current_function (tree fndecl)
 	  = save_target_globals_default_opts ();
     }
 
+  /* We don't know yet the optimize_size for this function.  */
+  DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
+
   arm_option_params_internal ();
 }
 

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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

The ARM target can switch different alignment requirements between the 
thumb or arm, thanks to the attribute ((target)). Using 
FUNCTION_BOUNDARY that now depends on the switchable target_flag.

The previous attempt to fix this was to use the set_current_function 
hook to reset DECL_ALIGN. On a second thought I found this not 
satisfactory because this hook is called multiple time between passes, 
whereas the setting only needs to be done once.

Instead, this patch resets the function's DECL_ALIGN in 
allocate_struct_function, when not enforced by the user or the language, 
after the attributes are processed.

Tested for arm-none-eabi (with the 1/2 part 
https://gcc.gnu.org/ml/gcc-patches/2015-09/msg02198.html)

Bootstraped for x86_64-unknown-linux-gnu and tested (c+,c++,fortran)

Comments ? OK for trunk ?

thanks

Christian



[-- Attachment #2: align3.patch --]
[-- Type: text/x-patch, Size: 818 bytes --]

2015-10-07  Christian Bruel  <christian.bruel@st.com>

	PR target/67745
	* function.c (allocate_struct_function): Relayout function's alignment.

Index: gcc/function.c
===================================================================
--- gcc/function.c	(revision 228515)
+++ gcc/function.c	(working copy)
@@ -4840,6 +4840,12 @@ allocate_struct_function (tree fndecl, b
 	  for (tree parm = DECL_ARGUMENTS (fndecl); parm;
 	       parm = DECL_CHAIN (parm))
 	    relayout_decl (parm);
+
+	  /* Similarly, relayout function's alignment if not forced.  */
+	  if (!DECL_USER_ALIGN (fndecl)
+	      && (TREE_CODE (fntype) != METHOD_TYPE
+		  || TARGET_PTRMEMFUNC_VBIT_LOCATION != ptrmemfunc_vbit_in_pfn))
+	    DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
 	}
 
       if (!abstract_p && aggregate_value_p (result, fndecl))

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/07/2015 09:04 AM, Christian Bruel wrote:
> +	  /* Similarly, relayout function's alignment if not forced.  */
> +	  if (!DECL_USER_ALIGN (fndecl)
> +	      && (TREE_CODE (fntype) != METHOD_TYPE
> +		  || TARGET_PTRMEMFUNC_VBIT_LOCATION != ptrmemfunc_vbit_in_pfn))
> +	    DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
>   	}

That's a very odd-looking condition. Why the vbit location test?


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

On 10/07/2015 12:18 PM, Bernd Schmidt wrote:
> On 10/07/2015 09:04 AM, Christian Bruel wrote:
>> +	  /* Similarly, relayout function's alignment if not forced.  */
>> +	  if (!DECL_USER_ALIGN (fndecl)
>> +	      && (TREE_CODE (fntype) != METHOD_TYPE
>> +		  || TARGET_PTRMEMFUNC_VBIT_LOCATION != ptrmemfunc_vbit_in_pfn))
>> +	    DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
>>    	}
>
> That's a very odd-looking condition. Why the vbit location test?

This is for member functions to make sure that the lsb address is 
reserved for the virtual function bit.

see cp/decl.c:

   /* If pointers to member functions use the least significant bit to
      indicate whether a function is virtual, ensure a pointer
      to this function will have that bit clear.  */
   if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
       && TREE_CODE (type) == METHOD_TYPE
       && DECL_ALIGN (decl) < 2 * BITS_PER_UNIT)
     DECL_ALIGN (decl) = 2 * BITS_PER_UNIT;

This happens for instance on i386 that aligns member functions on 2 
bytes, bigger than the one byte required boundary. So we cannot 
re-layout bellow that.

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/07/2015 12:45 PM, Christian Bruel wrote:
>
>
> On 10/07/2015 12:18 PM, Bernd Schmidt wrote:
>> On 10/07/2015 09:04 AM, Christian Bruel wrote:
>>> +      /* Similarly, relayout function's alignment if not forced.  */
>>> +      if (!DECL_USER_ALIGN (fndecl)
>>> +          && (TREE_CODE (fntype) != METHOD_TYPE
>>> +          || TARGET_PTRMEMFUNC_VBIT_LOCATION !=
>>> ptrmemfunc_vbit_in_pfn))
>>> +        DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
>>>        }
>>
>> That's a very odd-looking condition. Why the vbit location test?
>
> This is for member functions to make sure that the lsb address is
> reserved for the virtual function bit.
>
> see cp/decl.c:
>
>    /* If pointers to member functions use the least significant bit to
>       indicate whether a function is virtual, ensure a pointer
>       to this function will have that bit clear.  */
>    if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>        && TREE_CODE (type) == METHOD_TYPE
>        && DECL_ALIGN (decl) < 2 * BITS_PER_UNIT)
>      DECL_ALIGN (decl) = 2 * BITS_PER_UNIT;
>
> This happens for instance on i386 that aligns member functions on 2
> bytes, bigger than the one byte required boundary. So we cannot
> re-layout bellow that.

Hmm, at least that would need to be spelled out in a comment, but I 
think this would better be abstracted as

#define MINIMUM_FUNCTION_BOUNDARY(FN) \
  ....
in a header file, and used in both places.


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/07/2015 07:37 PM, Bernd Schmidt wrote:
> On 10/07/2015 12:45 PM, Christian Bruel wrote:

>> On 10/07/2015 12:18 PM, Bernd Schmidt wrote:
>>> On 10/07/2015 09:04 AM, Christian Bruel wrote:
>>>> +      /* Similarly, relayout function's alignment if not forced.  */
>>>> +      if (!DECL_USER_ALIGN (fndecl)
>>>> +          && (TREE_CODE (fntype) != METHOD_TYPE
>>>> +          || TARGET_PTRMEMFUNC_VBIT_LOCATION !=
>>>> ptrmemfunc_vbit_in_pfn))
>>>> +        DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
>>>>        }

Hmm, more questions are coming to mind - are other places that set 
DECL_ALIGN for functions redundant after this change? It looks like the 
main place where it's set is make_node_stat where it's also set to 
FUNCTION_BOUNDARY. That's small enough not to bother changing it, but it 
raises another issue: we don't allocate a struct function for mere 
declarations, so does that mean they can have an incorrect DECL_ALIGN?


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

On 10/07/2015 07:50 PM, Bernd Schmidt wrote:
> On 10/07/2015 07:37 PM, Bernd Schmidt wrote:
>> On 10/07/2015 12:45 PM, Christian Bruel wrote:
>
>>> On 10/07/2015 12:18 PM, Bernd Schmidt wrote:
>>>> On 10/07/2015 09:04 AM, Christian Bruel wrote:
>>>>> +      /* Similarly, relayout function's alignment if not forced.  */
>>>>> +      if (!DECL_USER_ALIGN (fndecl)
>>>>> +          && (TREE_CODE (fntype) != METHOD_TYPE
>>>>> +          || TARGET_PTRMEMFUNC_VBIT_LOCATION !=
>>>>> ptrmemfunc_vbit_in_pfn))
>>>>> +        DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
>>>>>         }
>
> Hmm, more questions are coming to mind - are other places that set
> DECL_ALIGN for functions redundant after this change? It looks like the
> main place where it's set is make_node_stat where it's also set to
> FUNCTION_BOUNDARY. That's small enough not to bother changing it,

Yes it is. I thought about poisoning it but make_node_stat is used for 
synthesized functions as well and we need to keep the default correct.

> but it
> raises another issue: we don't allocate a struct function for mere
> declarations, so does that mean they can have an incorrect DECL_ALIGN?
>

Probably at the time of start_decl, because DECL_ALIGN will have the 
boundary given by the global target_flags at that time. But this 
shouldn't be a problem since what matters is the DECL_ALIGN recomputed 
with the definition when there is something to layout.

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/08/2015 03:14 PM, Christian Bruel wrote:

> Probably at the time of start_decl, because DECL_ALIGN will have the
> boundary given by the global target_flags at that time. But this
> shouldn't be a problem since what matters is the DECL_ALIGN recomputed
> with the definition when there is something to layout.

I'm not so sure. Don't we take DECL_ALIGN into account when optimizing 
things like alignment tests?


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

On 10/08/2015 03:20 PM, Bernd Schmidt wrote:
> On 10/08/2015 03:14 PM, Christian Bruel wrote:
>
>> Probably at the time of start_decl, because DECL_ALIGN will have the
>> boundary given by the global target_flags at that time. But this
>> shouldn't be a problem since what matters is the DECL_ALIGN recomputed
>> with the definition when there is something to layout.
>
> I'm not so sure. Don't we take DECL_ALIGN into account when optimizing
> things like alignment tests?
>
>

Humm, I don't know what kind of alignment optimization for functions we 
have based on a declaration only. greping DECL_ALIGN on functions there 
are some bits in the ipa-icf code that seems to merge code using this 
information, but I think we have a definition at that point.
but honestly, I'm very unfamiliar with this pass. Do you have something 
else in mind ?

> Bernd
>

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/08/2015 03:50 PM, Christian Bruel wrote:
> Humm, I don't know what kind of alignment optimization for functions we
> have based on a declaration only. greping DECL_ALIGN on functions there
> are some bits in the ipa-icf code that seems to merge code using this
> information, but I think we have a definition at that point.
> but honestly, I'm very unfamiliar with this pass. Do you have something
> else in mind ?

I had a vague memory of us optimizing that, but I can't find the code 
either and maybe it's just not there. That doesn't mean someone isn't 
going to add it in the future, and I'm uncomfortable leaving incorrect 
DECL_ALIGN values around.

It looks like rest_of_decl_compilation may be a good place to take care 
of declarations, but using FUNCTION_BOUNDARY is probably going to give 
the wrong results. So maybe a target hook, function_boundary_for_decl, 
defaulting to just returning FUNCTION_BOUNDARY? Eventually it could 
replace the macro entirely.


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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



On 10/08/2015 04:30 PM, Bernd Schmidt wrote:
> On 10/08/2015 03:50 PM, Christian Bruel wrote:
>> Humm, I don't know what kind of alignment optimization for functions we
>> have based on a declaration only. greping DECL_ALIGN on functions there
>> are some bits in the ipa-icf code that seems to merge code using this
>> information, but I think we have a definition at that point.
>> but honestly, I'm very unfamiliar with this pass. Do you have something
>> else in mind ?
>
> I had a vague memory of us optimizing that, but I can't find the code
> either and maybe it's just not there. That doesn't mean someone isn't
> going to add it in the future, and I'm uncomfortable leaving incorrect
> DECL_ALIGN values around.
>
> It looks like rest_of_decl_compilation may be a good place to take care
> of declarations, but using FUNCTION_BOUNDARY is probably going to give
> the wrong results. So maybe a target hook, function_boundary_for_decl,
> defaulting to just returning FUNCTION_BOUNDARY? Eventually it could
> replace the macro entirely.
>
>

yes I see, I was hoping to avoid a new hook, but as you said it seems 
mandatory for the mere declaration case.

Here is one proposal, it defaults to nothing and the ARM implementation 
does not need to handle the vptr bit setting. so that simplifies a lot 
the things.

The hook is called from rest_of_decl_compilation for mere declarations 
and allocate_struct_function for definitions.







[-- Attachment #2: align_hook.patch --]
[-- Type: text/x-patch, Size: 7362 bytes --]

2015-09-29  Christian Bruel  <christian.bruel@st.com>

	PR target/67745
	* config/arm/arm.h (FUNCTION_BOUNDARY): Use FUNCTION_BOUNDARY_P.
	(FUNCTION_BOUNDARY_P): New macro:
	* config/arm/arm.c (TARGET_RELAYOUT_FUNCTION, arm_relayout_function): 
	New hook.
	* doc/tm.texi.in (TARGET_RELAYOUT_FUNCTION): Document.
	* doc/tm.texi (TARGET_RELAYOUT_FUNCTION): New hook.
	* gcc/target.def (TARGET_RELAYOUT_FUNCTION): Likewise.
	* gcc/function.c (allocate_struct_function): Call relayout_function hook.
	* gcc/passes.c (rest_of_decl_compilation): Likewise.
	* gcc/targhooks.c (default_relayout_function): New function.
	* gcc/targhooks.h (default_relayout_function): Declare.

--- gnu_trunk.p0/gcc/gcc/config/arm/arm.c	2015-10-12 10:34:27.599740376 +0200
+++ gnu_trunk.devs/gcc/gcc/config/arm/arm.c	2015-10-12 12:26:51.607398021 +0200
@@ -250,6 +250,7 @@ static void arm_override_options_after_c
 static void arm_option_print (FILE *, int, struct cl_target_option *);
 static void arm_set_current_function (tree);
 static bool arm_can_inline_p (tree, tree);
+static void arm_relayout_function (tree);
 static bool arm_valid_target_attribute_p (tree, tree, tree, int);
 static unsigned HOST_WIDE_INT arm_shift_truncation_mask (machine_mode);
 static bool arm_macro_fusion_p (void);
@@ -405,6 +406,9 @@ static const struct attribute_spec arm_a
 #undef TARGET_CAN_INLINE_P
 #define TARGET_CAN_INLINE_P arm_can_inline_p
 
+#undef TARGET_RELAYOUT_FUNCTION
+#define TARGET_RELAYOUT_FUNCTION arm_relayout_function
+
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE arm_option_override
 
@@ -29825,6 +29829,23 @@ arm_can_inline_p (tree caller ATTRIBUTE_
   return true;
 }
 
+/* Hook to fix function's alignment affected by target attribute.  */
+
+static void
+arm_relayout_function (tree fndecl)
+{
+  if (DECL_USER_ALIGN (fndecl))
+    return;
+
+  tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
+
+  if (!callee_tree)
+    callee_tree = target_option_default_node;
+
+  DECL_ALIGN (fndecl) =
+    FUNCTION_BOUNDARY_P (TREE_TARGET_OPTION (callee_tree)->x_target_flags);
+}
+
 /* Inner function to process the attribute((target(...))), take an argument and
    set the current options from the argument.  If we have a list, recursively
    go over the list.  */
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/config/arm/arm.h gnu_trunk.devs/gcc/gcc/config/arm/arm.h
--- gnu_trunk.p0/gcc/gcc/config/arm/arm.h	2015-10-12 10:34:27.607740391 +0200
+++ gnu_trunk.devs/gcc/gcc/config/arm/arm.h	2015-10-12 12:27:55.507546958 +0200
@@ -565,7 +565,8 @@ extern int arm_arch_crc;
 #define PREFERRED_STACK_BOUNDARY \
     (arm_abi == ARM_ABI_ATPCS ? 64 : STACK_BOUNDARY)
 
-#define FUNCTION_BOUNDARY           (TARGET_THUMB ? 16 : 32)
+#define FUNCTION_BOUNDARY_P(flags)  (TARGET_THUMB_P (flags) ? 16 : 32)
+#define FUNCTION_BOUNDARY           (FUNCTION_BOUNDARY_P (target_flags))
 
 /* The lowest bit is used to indicate Thumb-mode functions, so the
    vbit must go into the delta field of pointers to member
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/doc/tm.texi gnu_trunk.devs/gcc/gcc/doc/tm.texi
--- gnu_trunk.p0/gcc/gcc/doc/tm.texi	2015-10-12 10:33:29.907630642 +0200
+++ gnu_trunk.devs/gcc/gcc/doc/tm.texi	2015-10-12 12:33:33.880332253 +0200
@@ -9985,6 +9985,10 @@ default, inlining is not allowed if the
 specific target options and the caller does not use the same options.
 @end deftypefn
 
+@deftypefn {Target Hook} void TARGET_RELAYOUT_FUNCTION (tree @var{fndecl})
+This target hook fixes function @var{fndecl} after attributes are processed. Default does nothing. On ARM, the default function's alignment is updated with the attribute target.
+@end deftypefn
+
 @node Emulated TLS
 @section Emulating TLS
 @cindex Emulated TLS
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/doc/tm.texi.in gnu_trunk.devs/gcc/gcc/doc/tm.texi.in
--- gnu_trunk.p0/gcc/gcc/doc/tm.texi.in	2015-10-12 10:33:29.919630666 +0200
+++ gnu_trunk.devs/gcc/gcc/doc/tm.texi.in	2015-10-12 11:28:16.350590629 +0200
@@ -7274,6 +7274,8 @@ on this implementation detail.
 
 @hook TARGET_CAN_INLINE_P
 
+@hook TARGET_RELAYOUT_FUNCTION
+
 @node Emulated TLS
 @section Emulating TLS
 @cindex Emulated TLS
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/function.c gnu_trunk.devs/gcc/gcc/function.c
--- gnu_trunk.p0/gcc/gcc/function.c	2015-10-12 10:33:32.715635978 +0200
+++ gnu_trunk.devs/gcc/gcc/function.c	2015-10-12 10:04:42.764482359 +0200
@@ -4840,6 +4840,9 @@ allocate_struct_function (tree fndecl, b
 	  for (tree parm = DECL_ARGUMENTS (fndecl); parm;
 	       parm = DECL_CHAIN (parm))
 	    relayout_decl (parm);
+
+	  /* Similarly relayout the function decl.  */
+	  targetm.target_option.relayout_function (fndecl);
 	}
 
       if (!abstract_p && aggregate_value_p (result, fndecl))
--- gnu_trunk.p0/gcc/gcc/passes.c	2015-10-05 14:22:31.082400039 +0200
+++ gnu_trunk.devs/gcc/gcc/passes.c	2015-10-12 11:31:55.343150983 +0200
@@ -253,6 +253,11 @@ rest_of_decl_compilation (tree decl,
 	}
 #endif
 
+      /* Now that we have activated any function-specific attributes
+	 that might affect function decl, particularly align, relayout it.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL)
+	targetm.target_option.relayout_function (decl);
+
       timevar_pop (TV_VARCONST);
     }
   else if (TREE_CODE (decl) == TYPE_DECL
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/target.def gnu_trunk.devs/gcc/gcc/target.def
--- gnu_trunk.p0/gcc/gcc/target.def	2015-10-05 14:22:20.142374948 +0200
+++ gnu_trunk.devs/gcc/gcc/target.def	2015-10-12 12:34:23.744447530 +0200
@@ -5620,6 +5620,12 @@ specific target options and the caller d
  bool, (tree caller, tree callee),
  default_target_can_inline_p)
 
+DEFHOOK
+(relayout_function,
+"This target hook fixes function @var{fndecl} after attributes are processed. Default does nothing. On ARM, the default function's alignment is updated with the attribute target.",
+ void, (tree fndecl),
+ default_relayout_function)
+
 HOOK_VECTOR_END (target_option)
 
 /* For targets that need to mark extra registers as live on entry to
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/targhooks.c gnu_trunk.devs/gcc/gcc/targhooks.c
--- gnu_trunk.p0/gcc/gcc/targhooks.c	2015-08-25 10:28:00.504946542 +0200
+++ gnu_trunk.devs/gcc/gcc/targhooks.c	2015-10-12 11:24:50.866063124 +0200
@@ -1311,6 +1311,11 @@ default_target_option_pragma_parse (tree
   return false;
 }
 
+void
+default_relayout_function (tree fndecl)
+{
+}
+
 bool
 default_target_can_inline_p (tree caller, tree callee)
 {
diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/targhooks.h gnu_trunk.devs/gcc/gcc/targhooks.h
--- gnu_trunk.p0/gcc/gcc/targhooks.h	2015-10-05 14:22:17.774369515 +0200
+++ gnu_trunk.devs/gcc/gcc/targhooks.h	2015-10-12 11:24:40.914042025 +0200
@@ -161,6 +161,7 @@ extern bool default_hard_regno_scratch_o
 extern bool default_mode_dependent_address_p (const_rtx, addr_space_t);
 extern bool default_target_option_valid_attribute_p (tree, tree, tree, int);
 extern bool default_target_option_pragma_parse (tree, tree);
+extern void default_relayout_function (tree);
 extern bool default_target_can_inline_p (tree, tree);
 extern bool default_valid_pointer_mode (machine_mode);
 extern bool default_ref_may_alias_errno (struct ao_ref *);

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/12/2015 12:56 PM, Christian Bruel wrote:
> yes I see, I was hoping to avoid a new hook, but as you said it seems
> mandatory for the mere declaration case.
>
> Here is one proposal, it defaults to nothing and the ARM implementation
> does not need to handle the vptr bit setting. so that simplifies a lot
> the things.
>
> The hook is called from rest_of_decl_compilation for mere declarations
> and allocate_struct_function for definitions.

This looks good to me. I still think we also want your vptr patch.


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

On 10/12/2015 01:19 PM, Bernd Schmidt wrote:
> On 10/12/2015 12:56 PM, Christian Bruel wrote:
>> yes I see, I was hoping to avoid a new hook, but as you said it seems
>> mandatory for the mere declaration case.
>>
>> Here is one proposal, it defaults to nothing and the ARM implementation
>> does not need to handle the vptr bit setting. so that simplifies a lot
>> the things.
>>
>> The hook is called from rest_of_decl_compilation for mere declarations
>> and allocate_struct_function for definitions.
>
> This looks good to me. I still think we also want your vptr patch.

thanks. Will wait the ARM part review now.

I'm preparing the vptr patch separately since it's more related to 
cleanup than functionality, and doesn't need to be hooked.


>
>
> Bernd
>

refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks

[Christian Bruel]

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

Hello,

On 10/12/2015 01:19 PM, Bernd Schmidt wrote:
> On 10/12/2015 12:56 PM, Christian Bruel wrote:
>> yes I see, I was hoping to avoid a new hook, but as you said it seems
>> mandatory for the mere declaration case.
>>
>> Here is one proposal, it defaults to nothing and the ARM implementation
>> does not need to handle the vptr bit setting. so that simplifies a lot
>> the things.
>>
>> The hook is called from rest_of_decl_compilation for mere declarations
>> and allocate_struct_function for definitions.
>
> This looks good to me. I still think we also want your vptr patch.
>
 > Bernd
 >

Here is the "vptr patch".

2 changes to watch for the review :

  - I replaced the DECL_ALIGN checks by FUNCTION_BOUNDARY because there 
is no user align at that time of processing and it's consistent with the 
default TARGET_PTRMEMFUNC_VBIT_LOCATION macro implementation (or 
TARGET_PTRMEMFUNC_VBIT_LOCATION should also be function of DECL_ALIGN)

  - The second chunk from cp/lambda.c did not seem to be needed (?).
Thunks are FUNCTION_TYPE and I don't see why they would need to carry 
the vptr bit information (for METHOD_TYPE from my understanding). Or did 
I miss something here ?

bootstrapped on x86. no c/c++ new failures for
   x86 (ptrmemfunc_vbit_in_pfn, align 8)
   arm (ptrmemfunc_vbit_in_delta, align 16 or 32)
   sh4 (ptrmemfunc_vbit_in_pfn, align 16)

It's a code cleanup/refactoring. No new testcase.

thanks

[-- Attachment #2: align5.patch --]
[-- Type: text/x-patch, Size: 4934 bytes --]

2015-09-29  Christian Bruel  <christian.bruel@st.com>

	* function.h (MINIMUM_METHOD_BOUNDARY): New macro.
	* cp/decl.c (grokfndecl): Set DECL_ALIGN with MINIMUM_METHOD_BOUNDARY.
	* cp/method.c (implicitly_declare_fn): Likewise.
	* cp/lambda.c (maybe_add_lambda_conv_op): Likewise, remove wrong setting.
	* java/class.c (add_method_1): Likewise.



	  
Index: cp/decl.c
===================================================================
--- cp/decl.c	(revision 228756)
+++ cp/decl.c	(working copy)
@@ -7826,6 +7826,8 @@ grokfndecl (tree ctype,
       parm = build_this_parm (type, quals);
       DECL_CHAIN (parm) = parms;
       parms = parm;
+ 
+      /* Allocate space to hold the vptr bit if needed.  */
+      DECL_ALIGN (decl) = MINIMUM_METHOD_BOUNDARY;
     }
   DECL_ARGUMENTS (decl) = parms;
   for (t = parms; t; t = DECL_CHAIN (t))
@@ -7849,14 +7851,6 @@ grokfndecl (tree ctype,
       break;
     }
 
-  /* If pointers to member functions use the least significant bit to
-     indicate whether a function is virtual, ensure a pointer
-     to this function will have that bit clear.  */
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && TREE_CODE (type) == METHOD_TYPE
-      && DECL_ALIGN (decl) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (decl) = 2 * BITS_PER_UNIT;
-
   if (friendp
       && TREE_CODE (orig_declarator) == TEMPLATE_ID_EXPR)
     {
Index: cp/lambda.c
===================================================================
--- cp/lambda.c	(revision 228756)
+++ cp/lambda.c	(working copy)
@@ -1006,11 +1006,8 @@ maybe_add_lambda_conv_op (tree type)
   tree convfn = build_lang_decl (FUNCTION_DECL, name, fntype);
   tree fn = convfn;
   DECL_SOURCE_LOCATION (fn) = DECL_SOURCE_LOCATION (callop);
-
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
-
+  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
   SET_OVERLOADED_OPERATOR_CODE (fn, TYPE_EXPR);
   grokclassfn (type, fn, NO_SPECIAL);
   set_linkage_according_to_type (type, fn);
@@ -1042,9 +1039,7 @@ maybe_add_lambda_conv_op (tree type)
   tree statfn = build_lang_decl (FUNCTION_DECL, name, stattype);
   fn = statfn;
   DECL_SOURCE_LOCATION (fn) = DECL_SOURCE_LOCATION (callop);
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
   grokclassfn (type, fn, NO_SPECIAL);
   set_linkage_according_to_type (type, fn);
   rest_of_decl_compilation (fn, toplevel_bindings_p (), at_eof);
Index: cp/method.c
===================================================================
--- cp/method.c	(revision 228756)
+++ cp/method.c	(working copy)
@@ -1849,13 +1849,9 @@ implicitly_declare_fn (special_function_
       DECL_ASSIGNMENT_OPERATOR_P (fn) = 1;
       SET_OVERLOADED_OPERATOR_CODE (fn, NOP_EXPR);
     }
-  
-  /* If pointers to member functions use the least significant bit to
-     indicate whether a function is virtual, ensure a pointer
-     to this function will have that bit clear.  */
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
+
+  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
 
   /* Create the explicit arguments.  */
   if (rhs_parm_type)
Index: function.h
===================================================================
--- function.h	(revision 228756)
+++ function.h	(working copy)
@@ -537,6 +537,13 @@ do {								\
 #define ASLK_REDUCE_ALIGN 1
 #define ASLK_RECORD_PAD 2
 
+/* If pointers to member functions use the least significant bit to
+   indicate whether a function is virtual, ensure a pointer
+   to this function will have that bit clear.  */
+#define MINIMUM_METHOD_BOUNDARY \
+  ((TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn)	     \
+   ? MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNIT) : FUNCTION_BOUNDARY)
+
 \f
 
 extern void push_function_context (void);
Index: java/class.c
===================================================================
--- java/class.c	(revision 228756)
+++ java/class.c	(working copy)
@@ -779,13 +779,8 @@ add_method_1 (tree this_class, int acces
   DECL_CHAIN (fndecl) = TYPE_METHODS (this_class);
   TYPE_METHODS (this_class) = fndecl;
 
-  /* If pointers to member functions use the least significant bit to
-     indicate whether a function is virtual, ensure a pointer
-     to this function will have that bit clear.  */
-  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
-      && !(access_flags & ACC_STATIC)
-      && DECL_ALIGN (fndecl) < 2 * BITS_PER_UNIT)
-    DECL_ALIGN (fndecl) = 2 * BITS_PER_UNIT;
+  if (!(access_flags & ACC_STATIC))
+    DECL_ALIGN (fndecl) = MINIMUM_METHOD_BOUNDARY;
 
   /* Notice that this is a finalizer and update the class type
      accordingly. This is used to optimize instance allocation. */

Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks

[Bernd Schmidt]

On 10/16/2015 10:01 AM, Christian Bruel wrote:
> -
> -  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
> -      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
> -    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
> -
> +  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;

This looks like a change in behaviour. You want to use the max of M_M_B 
and the current alignment.


Bernd

Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks

[Christian Bruel]

On 10/16/2015 11:44 AM, Bernd Schmidt wrote:
> On 10/16/2015 10:01 AM, Christian Bruel wrote:
>> -
>> -  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>> -      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
>> -    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
>> -
>> +  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
>
> This looks like a change in behaviour. You want to use the max of M_M_B
> and the current alignment.
>
 > Bernd
 >

I'm not sure. at each point of the macro, we have the current alignment 
== FUNCTION_BOUNDARY, because we are just returning from the sequence 
build_lang_decl/make_node

so it looks like

   DECL_ALIGN (fn) = MAX (MINIMUM_METHOD_BOUNDARY, DECL_ALIGN (fn))

would be redundant with just

   DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY

did I miss something ?


>
> Bernd
>

Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks

[Christian Bruel]

On 10/16/2015 12:48 PM, Christian Bruel wrote:
>
>
> On 10/16/2015 11:44 AM, Bernd Schmidt wrote:
>> On 10/16/2015 10:01 AM, Christian Bruel wrote:
>>> -
>>> -  if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>>> -      && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
>>> -    DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
>>> -
>>> +  DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY;
>>
>> This looks like a change in behaviour. You want to use the max of M_M_B
>> and the current alignment.
>>
>  > Bernd
>  >
>
> I'm not sure. at each point of the macro, we have the current alignment
> == FUNCTION_BOUNDARY, because we are just returning from the sequence
> build_lang_decl/make_node
>
> so it looks like
>
>    DECL_ALIGN (fn) = MAX (MINIMUM_METHOD_BOUNDARY, DECL_ALIGN (fn))
>
> would be redundant with just
>
>    DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY
>
> did I miss something ?

maybe I realized that it was not clear that the MAX is part of the MMB 
macro that returns MAX (FUNCTION_BOUNDARY, 2 * BITS_PER_UNITS)


>
>
>>
>> Bernd
>>

Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks

[Bernd Schmidt]

On 10/16/2015 12:48 PM, Christian Bruel wrote:
> I'm not sure. at each point of the macro, we have the current alignment
> == FUNCTION_BOUNDARY, because we are just returning from the sequence
> build_lang_decl/make_node
>
> so it looks like
>
>    DECL_ALIGN (fn) = MAX (MINIMUM_METHOD_BOUNDARY, DECL_ALIGN (fn))
>
> would be redundant with just
>
>    DECL_ALIGN (fn) = MINIMUM_METHOD_BOUNDARY
>
> did I miss something ?

No, I managed to confuse myself. Patch is ok to install next Wednesday 
unless you hear otherwise from a C++ (or Java) maintainer.


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Ramana Radhakrishnan]

> 
> yes I see, I was hoping to avoid a new hook, but as you said it seems mandatory for the mere declaration case.
> 
> Here is one proposal, it defaults to nothing and the ARM implementation does not need to handle the vptr bit setting. so that simplifies a lot the things.
> 
> The hook is called from rest_of_decl_compilation for mere declarations and allocate_struct_function for definitions.


I'm not sure we have testsuite coverage for this -  can you add a test or 2 ?

> 
> 
> 
> 
> 
> 
> 
> align_hook.patch
> 
> 
> 2015-09-29  Christian Bruel  <christian.bruel@st.com>
> 
> 	PR target/67745
> 	* config/arm/arm.h (FUNCTION_BOUNDARY): Use FUNCTION_BOUNDARY_P.
> 	(FUNCTION_BOUNDARY_P): New macro:
> 	* config/arm/arm.c (TARGET_RELAYOUT_FUNCTION, arm_relayout_function): 
> 	New hook.

The ARM changes look ok to me. Please as usual watch out for any regressions.



> 	* doc/tm.texi.in (TARGET_RELAYOUT_FUNCTION): Document.
> 	* doc/tm.texi (TARGET_RELAYOUT_FUNCTION): New hook.
> 	* gcc/target.def (TARGET_RELAYOUT_FUNCTION): Likewise.
> 	* gcc/function.c (allocate_struct_function): Call relayout_function hook.
> 	* gcc/passes.c (rest_of_decl_compilation): Likewise.
> 	* gcc/targhooks.c (default_relayout_function): New function.
> 	* gcc/targhooks.h (default_relayout_function): Declare.
> 


> --- gnu_trunk.p0/gcc/gcc/config/arm/arm.c	2015-10-12 10:34:27.599740376 +0200
> +++ gnu_trunk.devs/gcc/gcc/config/arm/arm.c	2015-10-12 12:26:51.607398021 +0200
> @@ -250,6 +250,7 @@ static void arm_override_options_after_c
>  static void arm_option_print (FILE *, int, struct cl_target_option *);
>  static void arm_set_current_function (tree);
>  static bool arm_can_inline_p (tree, tree);
> +static void arm_relayout_function (tree);
>  static bool arm_valid_target_attribute_p (tree, tree, tree, int);
>  static unsigned HOST_WIDE_INT arm_shift_truncation_mask (machine_mode);
>  static bool arm_macro_fusion_p (void);
> @@ -405,6 +406,9 @@ static const struct attribute_spec arm_a
>  #undef TARGET_CAN_INLINE_P
>  #define TARGET_CAN_INLINE_P arm_can_inline_p
>  
> +#undef TARGET_RELAYOUT_FUNCTION
> +#define TARGET_RELAYOUT_FUNCTION arm_relayout_function
> +
>  #undef  TARGET_OPTION_OVERRIDE
>  #define TARGET_OPTION_OVERRIDE arm_option_override
>  
> @@ -29825,6 +29829,23 @@ arm_can_inline_p (tree caller ATTRIBUTE_
>    return true;
>  }
>  
> +/* Hook to fix function's alignment affected by target attribute.  */
> +
> +static void
> +arm_relayout_function (tree fndecl)
> +{
> +  if (DECL_USER_ALIGN (fndecl))
> +    return;
> +
> +  tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
> +
> +  if (!callee_tree)
> +    callee_tree = target_option_default_node;
> +
> +  DECL_ALIGN (fndecl) =
> +    FUNCTION_BOUNDARY_P (TREE_TARGET_OPTION (callee_tree)->x_target_flags);
> +}
> +
>  /* Inner function to process the attribute((target(...))), take an argument and
>     set the current options from the argument.  If we have a list, recursively
>     go over the list.  */
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/config/arm/arm.h gnu_trunk.devs/gcc/gcc/config/arm/arm.h
> --- gnu_trunk.p0/gcc/gcc/config/arm/arm.h	2015-10-12 10:34:27.607740391 +0200
> +++ gnu_trunk.devs/gcc/gcc/config/arm/arm.h	2015-10-12 12:27:55.507546958 +0200
> @@ -565,7 +565,8 @@ extern int arm_arch_crc;
>  #define PREFERRED_STACK_BOUNDARY \
>      (arm_abi == ARM_ABI_ATPCS ? 64 : STACK_BOUNDARY)
>  
> -#define FUNCTION_BOUNDARY           (TARGET_THUMB ? 16 : 32)
> +#define FUNCTION_BOUNDARY_P(flags)  (TARGET_THUMB_P (flags) ? 16 : 32)
> +#define FUNCTION_BOUNDARY           (FUNCTION_BOUNDARY_P (target_flags))
>  
>  /* The lowest bit is used to indicate Thumb-mode functions, so the
>     vbit must go into the delta field of pointers to member
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/doc/tm.texi gnu_trunk.devs/gcc/gcc/doc/tm.texi
> --- gnu_trunk.p0/gcc/gcc/doc/tm.texi	2015-10-12 10:33:29.907630642 +0200
> +++ gnu_trunk.devs/gcc/gcc/doc/tm.texi	2015-10-12 12:33:33.880332253 +0200
> @@ -9985,6 +9985,10 @@ default, inlining is not allowed if the
>  specific target options and the caller does not use the same options.
>  @end deftypefn
>  
> +@deftypefn {Target Hook} void TARGET_RELAYOUT_FUNCTION (tree @var{fndecl})
> +This target hook fixes function @var{fndecl} after attributes are processed. Default does nothing. On ARM, the default function's alignment is updated with the attribute target.
> +@end deftypefn
> +
>  @node Emulated TLS
>  @section Emulating TLS
>  @cindex Emulated TLS
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/doc/tm.texi.in gnu_trunk.devs/gcc/gcc/doc/tm.texi.in
> --- gnu_trunk.p0/gcc/gcc/doc/tm.texi.in	2015-10-12 10:33:29.919630666 +0200
> +++ gnu_trunk.devs/gcc/gcc/doc/tm.texi.in	2015-10-12 11:28:16.350590629 +0200
> @@ -7274,6 +7274,8 @@ on this implementation detail.
>  
>  @hook TARGET_CAN_INLINE_P
>  
> +@hook TARGET_RELAYOUT_FUNCTION
> +
>  @node Emulated TLS
>  @section Emulating TLS
>  @cindex Emulated TLS
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/function.c gnu_trunk.devs/gcc/gcc/function.c
> --- gnu_trunk.p0/gcc/gcc/function.c	2015-10-12 10:33:32.715635978 +0200
> +++ gnu_trunk.devs/gcc/gcc/function.c	2015-10-12 10:04:42.764482359 +0200
> @@ -4840,6 +4840,9 @@ allocate_struct_function (tree fndecl, b
>  	  for (tree parm = DECL_ARGUMENTS (fndecl); parm;
>  	       parm = DECL_CHAIN (parm))
>  	    relayout_decl (parm);
> +
> +	  /* Similarly relayout the function decl.  */
> +	  targetm.target_option.relayout_function (fndecl);
>  	}
>  
>        if (!abstract_p && aggregate_value_p (result, fndecl))
> --- gnu_trunk.p0/gcc/gcc/passes.c	2015-10-05 14:22:31.082400039 +0200
> +++ gnu_trunk.devs/gcc/gcc/passes.c	2015-10-12 11:31:55.343150983 +0200
> @@ -253,6 +253,11 @@ rest_of_decl_compilation (tree decl,
>  	}
>  #endif
>  
> +      /* Now that we have activated any function-specific attributes
> +	 that might affect function decl, particularly align, relayout it.  */
> +      if (TREE_CODE (decl) == FUNCTION_DECL)
> +	targetm.target_option.relayout_function (decl);
> +
>        timevar_pop (TV_VARCONST);
>      }
>    else if (TREE_CODE (decl) == TYPE_DECL
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/target.def gnu_trunk.devs/gcc/gcc/target.def
> --- gnu_trunk.p0/gcc/gcc/target.def	2015-10-05 14:22:20.142374948 +0200
> +++ gnu_trunk.devs/gcc/gcc/target.def	2015-10-12 12:34:23.744447530 +0200
> @@ -5620,6 +5620,12 @@ specific target options and the caller d
>   bool, (tree caller, tree callee),
>   default_target_can_inline_p)
>  
> +DEFHOOK
> +(relayout_function,
> +"This target hook fixes function @var{fndecl} after attributes are processed. Default does nothing. On ARM, the default function's alignment is updated with the attribute target.",
> + void, (tree fndecl),
> + default_relayout_function)

Can this just be hook_void_tree ? Then you don't need the empty default_relayout_function ?




> +
>  HOOK_VECTOR_END (target_option)
>  
>  /* For targets that need to mark extra registers as live on entry to
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/targhooks.c gnu_trunk.devs/gcc/gcc/targhooks.c
> --- gnu_trunk.p0/gcc/gcc/targhooks.c	2015-08-25 10:28:00.504946542 +0200
> +++ gnu_trunk.devs/gcc/gcc/targhooks.c	2015-10-12 11:24:50.866063124 +0200
> @@ -1311,6 +1311,11 @@ default_target_option_pragma_parse (tree
>    return false;
>  }
>  
> +void
> +default_relayout_function (tree fndecl)
> +{
> +}
> +
>  bool
>  default_target_can_inline_p (tree caller, tree callee)
>  {
> diff '--exclude=.svn' '--exclude=*~' -r -up gnu_trunk.p0/gcc/gcc/targhooks.h gnu_trunk.devs/gcc/gcc/targhooks.h
> --- gnu_trunk.p0/gcc/gcc/targhooks.h	2015-10-05 14:22:17.774369515 +0200
> +++ gnu_trunk.devs/gcc/gcc/targhooks.h	2015-10-12 11:24:40.914042025 +0200
> @@ -161,6 +161,7 @@ extern bool default_hard_regno_scratch_o
>  extern bool default_mode_dependent_address_p (const_rtx, addr_space_t);
>  extern bool default_target_option_valid_attribute_p (tree, tree, tree, int);
>  extern bool default_target_option_pragma_parse (tree, tree);
> +extern void default_relayout_function (tree);
>  extern bool default_target_can_inline_p (tree, tree);
>  extern bool default_valid_pointer_mode (machine_mode);
>  extern bool default_ref_may_alias_errno (struct ao_ref *);
> 

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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

Hi Ramana,

On 10/13/2015 10:03 AM, Ramana Radhakrishnan wrote:
>
>>
>> yes I see, I was hoping to avoid a new hook, but as you said it seems mandatory for the mere declaration case.
>>
>> Here is one proposal, it defaults to nothing and the ARM implementation does not need to handle the vptr bit setting. so that simplifies a lot the things.
>>
>> The hook is called from rest_of_decl_compilation for mere declarations and allocate_struct_function for definitions.
>
>
> I'm not sure we have testsuite coverage for this -  can you add a test or 2 ?

there were in the first part of the patch set 1/1 
(https://gcc.gnu.org/ml/gcc-patches/2015-09/msg02198.html)

Here is another one to test more combinations.

[-- Attachment #2: attr-align.c --]
[-- Type: text/x-csrc, Size: 518 bytes --]

/* PR target/67745
   Verify alignment when both attribute optimize and target are used.  */
/* { dg-do compile } */
/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */

void
__attribute__ ((target ("arm")))
bar()
{
}

void
__attribute__ ((target ("thumb")))
__attribute__ ((optimize ("Os")))
foo()
{
}

void
__attribute__ ((target ("thumb")))
__attribute__ ((optimize ("O2")))
rab()
{
}

/* { dg-final { scan-assembler-times ".align\[ \t]2" 2 } } */
/* { dg-final { scan-assembler ".align\[ \t]1" } } */

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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

committed patch attached.
with the default_relayout_function replaced by hook_void_tree as 
suggested by Ramana,



[-- Attachment #2: commit_align1.patch --]
[-- Type: text/x-patch, Size: 11830 bytes --]

Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 228903)
+++ gcc/ChangeLog	(revision 228904)
@@ -1,3 +1,13 @@
+2015-10-16  Christian Bruel  <christian.bruel@st.com>
+
+	PR target/67745
+	* config/arm/arm.h (FUNCTION_BOUNDARY): Move optimize_size condition to:
+	* config/arm/arm.c (arm_option_override_internal): Call
+	arm_override_options_after_change_1.
+	(arm_override_options_after_change): New function.
+	(arm_override_options_after_change_1): Likewise.
+	(TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): Define hook.
+
 2015-10-11  Jan Hubicka  <hubicka@ucw.cz>
 
 	Revert:
Index: gcc/testsuite/gcc.target/arm/attr-align1.c
===================================================================
--- gcc/testsuite/gcc.target/arm/attr-align1.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/attr-align1.c	(revision 228904)
@@ -0,0 +1,27 @@
+/* PR target/67745
+   Verify alignment when both attribute optimize and target are used.  */
+/* { dg-do compile } */
+/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
+
+void
+__attribute__ ((target ("arm")))
+bar()
+{
+}
+
+void
+__attribute__ ((target ("thumb")))
+__attribute__ ((optimize ("Os")))
+foo()
+{
+}
+
+void
+__attribute__ ((target ("thumb")))
+__attribute__ ((optimize ("O2")))
+rab()
+{
+}
+
+/* { dg-final { scan-assembler-times ".align\[ \t]2" 2 } } */
+/* { dg-final { scan-assembler ".align\[ \t]1" } } */
Index: gcc/testsuite/gcc.target/arm/no-align.c
===================================================================
--- gcc/testsuite/gcc.target/arm/no-align.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/no-align.c	(revision 228904)
@@ -0,0 +1,12 @@
+/* PR target/67745
+   Verify that -mthumb code is not aligned.  */
+/* { dg-do compile } */
+/* { dg-options "-O2 -mthumb -fno-align-functions" }  */
+/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
+
+void
+foo()
+{
+}
+
+/* { dg-final { scan-assembler-not ".align\[ \t]2" } } */
Index: gcc/testsuite/gcc.target/arm/attr-align2.c
===================================================================
--- gcc/testsuite/gcc.target/arm/attr-align2.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/attr-align2.c	(revision 228904)
@@ -0,0 +1,15 @@
+/* PR target/67745
+   Verify alignment when attribute optimize is used.  */
+/* { dg-do compile } */
+/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
+/* { dg-options "-O2 -mthumb" }  */
+
+/* Check that thumb code is always 2 bytes aligned for -Os.  */
+
+void
+__attribute__ ((optimize("Os")))
+foo()
+{
+}
+
+/* { dg-final { scan-assembler ".align\[ \t]1" } } */
Index: gcc/testsuite/gcc.target/arm/attr-align3.c
===================================================================
--- gcc/testsuite/gcc.target/arm/attr-align3.c	(revision 0)
+++ gcc/testsuite/gcc.target/arm/attr-align3.c	(revision 228904)
@@ -0,0 +1,13 @@
+/* PR target/67745
+   Verify alignment when attribute target is used.  */
+/* { dg-do compile } */
+/* { dg-skip-if "" { ! { arm_thumb1_ok || arm_thumb2_ok } } } */
+/* { dg-options "-Os -mthumb" }  */
+
+/* Check that arm code is always 4 bytes aligned.  */
+void  __attribute__ ((target ("arm")))
+c(void)
+{
+}
+
+/* { dg-final { scan-assembler-not ".align\[ \t]1" } } */
Index: gcc/testsuite/ChangeLog
===================================================================
--- gcc/testsuite/ChangeLog	(revision 228903)
+++ gcc/testsuite/ChangeLog	(revision 228904)
@@ -1,3 +1,11 @@
+2015-10-16  Christian Bruel  <christian.bruel@st.com>
+
+	PR target/67745
+        * gcc.target/arm/no-align.c: New test.
+        * gcc.target/arm/attr-align1.c: New test.
+        * gcc.target/arm/attr-align2.c: New test.
+        * gcc.target/arm/attr-align3.c: New test.
+
 2015-10-11  Jan Hubicka  <hubicka@ucw.cz>
 
 	* gcc.c-torture/compile/icfmatch.c: Add testcase
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 228903)
+++ gcc/config/arm/arm.c	(revision 228904)
@@ -246,6 +246,7 @@
 static void arm_expand_builtin_va_start (tree, rtx);
 static tree arm_gimplify_va_arg_expr (tree, tree, gimple_seq *, gimple_seq *);
 static void arm_option_override (void);
+static void arm_override_options_after_change (void);
 static void arm_option_print (FILE *, int, struct cl_target_option *);
 static void arm_set_current_function (tree);
 static bool arm_can_inline_p (tree, tree);
@@ -407,6 +408,9 @@
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE arm_option_override
 
+#undef TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE
+#define TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE arm_override_options_after_change
+
 #undef TARGET_OPTION_PRINT
 #define TARGET_OPTION_PRINT arm_option_print
 
@@ -2810,11 +2814,29 @@
 /* Options after initial target override.  */
 static GTY(()) tree init_optimize;
 
+static void
+arm_override_options_after_change_1 (struct gcc_options *opts)
+{
+  if (opts->x_align_functions <= 0)
+    opts->x_align_functions = TARGET_THUMB_P (opts->x_target_flags)
+      && opts->x_optimize_size ? 2 : 4;
+}
+
+/* Implement targetm.override_options_after_change.  */
+
+static void
+arm_override_options_after_change (void)
+{
+  arm_override_options_after_change_1 (&global_options);
+}
+
 /* Reset options between modes that the user has specified.  */
 static void
 arm_option_override_internal (struct gcc_options *opts,
 			      struct gcc_options *opts_set)
 {
+  arm_override_options_after_change_1 (opts);
+
   if (TARGET_THUMB_P (opts->x_target_flags)
       && !(ARM_FSET_HAS_CPU1 (insn_flags, FL_THUMB)))
     {
Index: gcc/config/arm/arm.h
===================================================================
--- gcc/config/arm/arm.h	(revision 228903)
+++ gcc/config/arm/arm.h	(revision 228904)
@@ -565,7 +565,7 @@
 #define PREFERRED_STACK_BOUNDARY \
     (arm_abi == ARM_ABI_ATPCS ? 64 : STACK_BOUNDARY)
 
-#define FUNCTION_BOUNDARY  ((TARGET_THUMB && optimize_size) ? 16 : 32)
+#define FUNCTION_BOUNDARY           (TARGET_THUMB ? 16 : 32)
 
 /* The lowest bit is used to indicate Thumb-mode functions, so the
    vbit must go into the delta field of pointers to member
Index: gcc/doc/tm.texi
===================================================================
--- gcc/doc/tm.texi	(revision 228911)
+++ gcc/doc/tm.texi	(revision 228912)
@@ -9985,6 +9985,10 @@
 specific target options and the caller does not use the same options.
 @end deftypefn
 
+@deftypefn {Target Hook} void TARGET_RELAYOUT_FUNCTION (tree @var{fndecl})
+This target hook fixes function @var{fndecl} after attributes are processed. Default does nothing. On ARM, the default function's alignment is updated with the attribute target.
+@end deftypefn
+
 @node Emulated TLS
 @section Emulating TLS
 @cindex Emulated TLS
Index: gcc/doc/tm.texi.in
===================================================================
--- gcc/doc/tm.texi.in	(revision 228911)
+++ gcc/doc/tm.texi.in	(revision 228912)
@@ -7274,6 +7274,8 @@
 
 @hook TARGET_CAN_INLINE_P
 
+@hook TARGET_RELAYOUT_FUNCTION
+
 @node Emulated TLS
 @section Emulating TLS
 @cindex Emulated TLS
Index: gcc/target.def
===================================================================
--- gcc/target.def	(revision 228911)
+++ gcc/target.def	(revision 228912)
@@ -5620,6 +5620,12 @@
  bool, (tree caller, tree callee),
  default_target_can_inline_p)
 
+DEFHOOK
+(relayout_function,
+"This target hook fixes function @var{fndecl} after attributes are processed. Default does nothing. On ARM, the default function's alignment is updated with the attribute target.",
+ void, (tree fndecl),
+ hook_void_tree)
+
 HOOK_VECTOR_END (target_option)
 
 /* For targets that need to mark extra registers as live on entry to
Index: gcc/ChangeLog
===================================================================
--- gcc/ChangeLog	(revision 228911)
+++ gcc/ChangeLog	(revision 228912)
@@ -1,5 +1,18 @@
 2015-10-16  Christian Bruel  <christian.bruel@st.com>
 
+ 	PR target/67745
+ 	* config/arm/arm.h (FUNCTION_BOUNDARY): Use FUNCTION_BOUNDARY_P.
+ 	(FUNCTION_BOUNDARY_P): New macro:
+ 	* config/arm/arm.c (TARGET_RELAYOUT_FUNCTION, arm_relayout_function): 
+ 	New hook.
+ 	* doc/tm.texi.in (TARGET_RELAYOUT_FUNCTION): Document.
+ 	* doc/tm.texi (TARGET_RELAYOUT_FUNCTION): New hook.
+ 	* gcc/target.def (TARGET_RELAYOUT_FUNCTION): Likewise.
+ 	* gcc/function.c (allocate_struct_function): Call relayout_function hook.
+ 	* gcc/passes.c (rest_of_decl_compilation): Likewise.
+
+2015-10-16  Christian Bruel  <christian.bruel@st.com>
+
 	PR target/67745
 	* config/arm/arm.h (FUNCTION_BOUNDARY): Move optimize_size condition to:
 	* config/arm/arm.c (arm_option_override_internal): Call
Index: gcc/function.c
===================================================================
--- gcc/function.c	(revision 228911)
+++ gcc/function.c	(revision 228912)
@@ -4840,6 +4840,9 @@
 	  for (tree parm = DECL_ARGUMENTS (fndecl); parm;
 	       parm = DECL_CHAIN (parm))
 	    relayout_decl (parm);
+
+	  /* Similarly relayout the function decl.  */
+	  targetm.target_option.relayout_function (fndecl);
 	}
 
       if (!abstract_p && aggregate_value_p (result, fndecl))
Index: gcc/passes.c
===================================================================
--- gcc/passes.c	(revision 228911)
+++ gcc/passes.c	(revision 228912)
@@ -253,6 +253,11 @@
 	}
 #endif
 
+      /* Now that we have activated any function-specific attributes
+	 that might affect function decl, particularly align, relayout it.  */
+      if (TREE_CODE (decl) == FUNCTION_DECL)
+	targetm.target_option.relayout_function (decl);
+
       timevar_pop (TV_VARCONST);
     }
   else if (TREE_CODE (decl) == TYPE_DECL
Index: gcc/config/arm/arm.c
===================================================================
--- gcc/config/arm/arm.c	(revision 228911)
+++ gcc/config/arm/arm.c	(revision 228912)
@@ -250,6 +250,7 @@
 static void arm_option_print (FILE *, int, struct cl_target_option *);
 static void arm_set_current_function (tree);
 static bool arm_can_inline_p (tree, tree);
+static void arm_relayout_function (tree);
 static bool arm_valid_target_attribute_p (tree, tree, tree, int);
 static unsigned HOST_WIDE_INT arm_shift_truncation_mask (machine_mode);
 static bool arm_macro_fusion_p (void);
@@ -405,6 +406,9 @@
 #undef TARGET_CAN_INLINE_P
 #define TARGET_CAN_INLINE_P arm_can_inline_p
 
+#undef TARGET_RELAYOUT_FUNCTION
+#define TARGET_RELAYOUT_FUNCTION arm_relayout_function
+
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE arm_option_override
 
@@ -29825,6 +29829,23 @@
   return true;
 }
 
+/* Hook to fix function's alignment affected by target attribute.  */
+
+static void
+arm_relayout_function (tree fndecl)
+{
+  if (DECL_USER_ALIGN (fndecl))
+    return;
+
+  tree callee_tree = DECL_FUNCTION_SPECIFIC_TARGET (fndecl);
+
+  if (!callee_tree)
+    callee_tree = target_option_default_node;
+
+  DECL_ALIGN (fndecl) =
+    FUNCTION_BOUNDARY_P (TREE_TARGET_OPTION (callee_tree)->x_target_flags);
+}
+
 /* Inner function to process the attribute((target(...))), take an argument and
    set the current options from the argument.  If we have a list, recursively
    go over the list.  */
Index: gcc/config/arm/arm.h
===================================================================
--- gcc/config/arm/arm.h	(revision 228911)
+++ gcc/config/arm/arm.h	(revision 228912)
@@ -565,7 +565,8 @@
 #define PREFERRED_STACK_BOUNDARY \
     (arm_abi == ARM_ABI_ATPCS ? 64 : STACK_BOUNDARY)
 
-#define FUNCTION_BOUNDARY           (TARGET_THUMB ? 16 : 32)
+#define FUNCTION_BOUNDARY_P(flags)  (TARGET_THUMB_P (flags) ? 16 : 32)
+#define FUNCTION_BOUNDARY           (FUNCTION_BOUNDARY_P (target_flags))
 
 /* The lowest bit is used to indicate Thumb-mode functions, so the
    vbit must go into the delta field of pointers to member

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Christian Bruel]

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



On 10/07/2015 07:37 PM, Bernd Schmidt wrote:
> On 10/07/2015 12:45 PM, Christian Bruel wrote:
>>
>>
>> On 10/07/2015 12:18 PM, Bernd Schmidt wrote:
>>> On 10/07/2015 09:04 AM, Christian Bruel wrote:
>>>> +      /* Similarly, relayout function's alignment if not forced.  */
>>>> +      if (!DECL_USER_ALIGN (fndecl)
>>>> +          && (TREE_CODE (fntype) != METHOD_TYPE
>>>> +          || TARGET_PTRMEMFUNC_VBIT_LOCATION !=
>>>> ptrmemfunc_vbit_in_pfn))
>>>> +        DECL_ALIGN (fndecl) = FUNCTION_BOUNDARY;
>>>>         }
>>>
>>> That's a very odd-looking condition. Why the vbit location test?
>>
>> This is for member functions to make sure that the lsb address is
>> reserved for the virtual function bit.
>>
>> see cp/decl.c:
>>
>>     /* If pointers to member functions use the least significant bit to
>>        indicate whether a function is virtual, ensure a pointer
>>        to this function will have that bit clear.  */
>>     if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>>         && TREE_CODE (type) == METHOD_TYPE
>>         && DECL_ALIGN (decl) < 2 * BITS_PER_UNIT)
>>       DECL_ALIGN (decl) = 2 * BITS_PER_UNIT;
>>
>> This happens for instance on i386 that aligns member functions on 2
>> bytes, bigger than the one byte required boundary. So we cannot
>> re-layout bellow that.
>
> Hmm, at least that would need to be spelled out in a comment, but I
> think this would better be abstracted as
>
> #define MINIMUM_FUNCTION_BOUNDARY(FN) \
>    ....
> in a header file, and used in both places.
>

OK, Similar pattern occurs at many other places, that changed also in 
the attached proposal.
Not fully tested (in particular the java part) and no ChangeLog. Just to 
make sure that we agree on the interface first.

Thanks

Christian

>
> Bernd
>

[-- Attachment #2: align5.patch --]
[-- Type: text/x-patch, Size: 3010 bytes --]

Index: gcc/builtins.c
===================================================================
304a305,306
>       tree fntype = exp ? TREE_TYPE (exp) : NULL_TREE;
> 
309,310c311,312
<       if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn)
< 	align = 2 * BITS_PER_UNIT;
---
>       align = MAX (minimum_function_boundary (exp), align);
> 
Index: gcc/cp/decl.c
===================================================================
7855,7858c7855
<   if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
<       && TREE_CODE (type) == METHOD_TYPE
<       && DECL_ALIGN (decl) < 2 * BITS_PER_UNIT)
<     DECL_ALIGN (decl) = 2 * BITS_PER_UNIT;
---
>   DECL_ALIGN (decl) = MAX (minimum_function_boundary (decl), DECL_ALIGN (decl));
Index: gcc/cp/lambda.c
===================================================================
1010,1012c1010,1011
<   if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
<       && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
<     DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
---
>   extern int need_vptr_align (void);
>   DECL_ALIGN (fn) = MAX (minimum_function_boundary (fn), DECL_ALIGN (fn));
1045,1047c1044
<   if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
<       && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
<     DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
---
>   DECL_ALIGN (fn) = MAX (minimum_function_boundary (fn), DECL_ALIGN (fn));
Index: gcc/cp/method.c
===================================================================
1852c1852
<   
---
> 
1856,1858c1856
<   if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
<       && DECL_ALIGN (fn) < 2 * BITS_PER_UNIT)
<     DECL_ALIGN (fn) = 2 * BITS_PER_UNIT;
---
>   DECL_ALIGN (fn) = MAX (minimum_function_boundary (fn), DECL_ALIGN (fn));
Index: gcc/function.c
===================================================================
4791a4792,4806
> unsigned int
> minimum_function_boundary (tree fndecl)
> {
>   gcc_assert (fndecl);
> 
>   tree fntype = TREE_TYPE (fndecl);
> 
>   if (TREE_CODE (fntype) == METHOD_TYPE
>       && TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
>       && FUNCTION_BOUNDARY < 2 * BITS_PER_UNIT)
>       return 2 * BITS_PER_UNIT;
> 
>   return FUNCTION_BOUNDARY;
> }
> 
4842a4858,4861
> 
> 	  /* Similarly, relayout function's alignment if not forced.  */
> 	  if (!DECL_USER_ALIGN (fndecl))
> 	    DECL_ALIGN (fndecl) = minimum_function_boundary (fndecl);
Index: gcc/function.h
===================================================================
601c601
< 
---
> extern unsigned int minimum_function_boundary (tree);
Index: gcc/java/class.c
===================================================================
785,788c785,786
<   if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn
<       && !(access_flags & ACC_STATIC)
<       && DECL_ALIGN (fndecl) < 2 * BITS_PER_UNIT)
<     DECL_ALIGN (fndecl) = 2 * BITS_PER_UNIT;
---
>   if !(access_flags & ACC_STATIC)
>     DECL_ALIGN (fndecl) = MAX (minimum_function_boundary (decl), DECL_ALIGN (fndecl));

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Bernd Schmidt]

On 10/08/2015 04:01 PM, Christian Bruel wrote:

> OK, Similar pattern occurs at many other places, that changed also in
> the attached proposal.
> Not fully tested (in particular the java part) and no ChangeLog. Just to
> make sure that we agree on the interface first.

That looks like a plain diff rather than context or unified, which is 
very hard to read, but I think this is the approach I was looking for. 
Well done spotting the other places.


Bernd

Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2

[Jeff Law]

On 10/07/2015 01:04 AM, Christian Bruel wrote:
> The ARM target can switch different alignment requirements between the
> thumb or arm, thanks to the attribute ((target)). Using
> FUNCTION_BOUNDARY that now depends on the switchable target_flag.
>
> The previous attempt to fix this was to use the set_current_function
> hook to reset DECL_ALIGN. On a second thought I found this not
> satisfactory because this hook is called multiple time between passes,
> whereas the setting only needs to be done once.
>
> Instead, this patch resets the function's DECL_ALIGN in
> allocate_struct_function, when not enforced by the user or the language,
> after the attributes are processed.
>
> Tested for arm-none-eabi (with the 1/2 part
> https://gcc.gnu.org/ml/gcc-patches/2015-09/msg02198.html)
>
> Bootstraped for x86_64-unknown-linux-gnu and tested (c+,c++,fortran)
>
> Comments ? OK for trunk ?
Bernd seems pretty engaged at this point.  I'll just add I like this 
much more than the prior approaches.

jeff