* [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
@ 2015-09-29 13:44 Christian Bruel
2015-09-29 13:59 ` Christian Bruel
2015-09-30 19:30 ` Jeff Law
0 siblings, 2 replies; 28+ messages in thread
From: Christian Bruel @ 2015-09-29 13:44 UTC (permalink / raw)
Cc: gcc-patches, kyrylo.tkachov, kyrylo.tkachov, richard.earnshaw
[-- 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,
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-09-29 13:44 [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Christian Bruel
@ 2015-09-29 13:59 ` Christian Bruel
2015-09-30 19:30 ` Jeff Law
1 sibling, 0 replies; 28+ messages in thread
From: Christian Bruel @ 2015-09-29 13:59 UTC (permalink / raw)
To: gcc-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-09-29 13:44 [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Christian Bruel
2015-09-29 13:59 ` Christian Bruel
@ 2015-09-30 19:30 ` Jeff Law
2015-10-01 7:12 ` Christian Bruel
1 sibling, 1 reply; 28+ messages in thread
From: Jeff Law @ 2015-09-30 19:30 UTC (permalink / raw)
To: Christian Bruel; +Cc: gcc-patches, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-09-30 19:30 ` Jeff Law
@ 2015-10-01 7:12 ` Christian Bruel
2015-10-01 16:11 ` Christian Bruel
0 siblings, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-01 7:12 UTC (permalink / raw)
To: Jeff Law; +Cc: gcc-patches, kyrylo.tkachov, richard.earnshaw
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,
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-01 7:12 ` Christian Bruel
@ 2015-10-01 16:11 ` Christian Bruel
2015-10-07 7:05 ` Christian Bruel
0 siblings, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-01 16:11 UTC (permalink / raw)
To: law; +Cc: gcc-patches
[-- 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 ();
}
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-01 16:11 ` Christian Bruel
@ 2015-10-07 7:05 ` Christian Bruel
2015-10-07 10:18 ` Bernd Schmidt
2015-10-07 17:40 ` Jeff Law
0 siblings, 2 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-07 7:05 UTC (permalink / raw)
To: law; +Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
[-- 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))
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 7:05 ` Christian Bruel
@ 2015-10-07 10:18 ` Bernd Schmidt
2015-10-07 10:45 ` Christian Bruel
2015-10-07 17:40 ` Jeff Law
1 sibling, 1 reply; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-07 10:18 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 10:18 ` Bernd Schmidt
@ 2015-10-07 10:45 ` Christian Bruel
2015-10-07 17:37 ` Bernd Schmidt
0 siblings, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-07 10:45 UTC (permalink / raw)
To: Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 10:45 ` Christian Bruel
@ 2015-10-07 17:37 ` Bernd Schmidt
2015-10-07 17:50 ` Bernd Schmidt
2015-10-08 14:01 ` Christian Bruel
0 siblings, 2 replies; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-07 17:37 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 7:05 ` Christian Bruel
2015-10-07 10:18 ` Bernd Schmidt
@ 2015-10-07 17:40 ` Jeff Law
1 sibling, 0 replies; 28+ messages in thread
From: Jeff Law @ 2015-10-07 17:40 UTC (permalink / raw)
To: Christian Bruel
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 17:37 ` Bernd Schmidt
@ 2015-10-07 17:50 ` Bernd Schmidt
2015-10-08 13:14 ` Christian Bruel
2015-10-08 14:01 ` Christian Bruel
1 sibling, 1 reply; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-07 17:50 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 17:50 ` Bernd Schmidt
@ 2015-10-08 13:14 ` Christian Bruel
2015-10-08 13:20 ` Bernd Schmidt
0 siblings, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-08 13:14 UTC (permalink / raw)
To: Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-08 13:14 ` Christian Bruel
@ 2015-10-08 13:20 ` Bernd Schmidt
2015-10-08 13:51 ` Christian Bruel
0 siblings, 1 reply; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-08 13:20 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-08 13:20 ` Bernd Schmidt
@ 2015-10-08 13:51 ` Christian Bruel
2015-10-08 14:30 ` Bernd Schmidt
0 siblings, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-08 13:51 UTC (permalink / raw)
To: Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-07 17:37 ` Bernd Schmidt
2015-10-07 17:50 ` Bernd Schmidt
@ 2015-10-08 14:01 ` Christian Bruel
2015-10-08 14:05 ` Bernd Schmidt
1 sibling, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-08 14:01 UTC (permalink / raw)
To: Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
[-- 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));
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-08 14:01 ` Christian Bruel
@ 2015-10-08 14:05 ` Bernd Schmidt
0 siblings, 0 replies; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-08 14:05 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-08 13:51 ` Christian Bruel
@ 2015-10-08 14:30 ` Bernd Schmidt
2015-10-12 10:56 ` Christian Bruel
0 siblings, 1 reply; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-08 14:30 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-08 14:30 ` Bernd Schmidt
@ 2015-10-12 10:56 ` Christian Bruel
2015-10-12 11:19 ` Bernd Schmidt
2015-10-13 8:03 ` [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Ramana Radhakrishnan
0 siblings, 2 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-12 10:56 UTC (permalink / raw)
To: Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
[-- 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 *);
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-12 10:56 ` Christian Bruel
@ 2015-10-12 11:19 ` Bernd Schmidt
2015-10-12 11:26 ` Christian Bruel
2015-10-16 8:03 ` refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks Christian Bruel
2015-10-13 8:03 ` [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Ramana Radhakrishnan
1 sibling, 2 replies; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-12 11:19 UTC (permalink / raw)
To: Christian Bruel, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-12 11:19 ` Bernd Schmidt
@ 2015-10-12 11:26 ` Christian Bruel
2015-10-16 8:03 ` refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks Christian Bruel
1 sibling, 0 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-12 11:26 UTC (permalink / raw)
To: Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-12 10:56 ` Christian Bruel
2015-10-12 11:19 ` Bernd Schmidt
@ 2015-10-13 8:03 ` Ramana Radhakrishnan
2015-10-13 10:18 ` Christian Bruel
2015-10-16 14:18 ` Christian Bruel
1 sibling, 2 replies; 28+ messages in thread
From: Ramana Radhakrishnan @ 2015-10-13 8:03 UTC (permalink / raw)
To: Christian Bruel, Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
>
> 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 *);
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-13 8:03 ` [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Ramana Radhakrishnan
@ 2015-10-13 10:18 ` Christian Bruel
2015-10-16 14:18 ` Christian Bruel
1 sibling, 0 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-13 10:18 UTC (permalink / raw)
To: Ramana Radhakrishnan, Bernd Schmidt, law
Cc: gcc-patches, Ramana.Radhakrishnan, kyrylo.tkachov, richard.earnshaw
[-- 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" } } */
^ permalink raw reply [flat|nested] 28+ messages in thread
* refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks
2015-10-12 11:19 ` Bernd Schmidt
2015-10-12 11:26 ` Christian Bruel
@ 2015-10-16 8:03 ` Christian Bruel
2015-10-16 9:47 ` Bernd Schmidt
1 sibling, 1 reply; 28+ messages in thread
From: Christian Bruel @ 2015-10-16 8:03 UTC (permalink / raw)
To: Bernd Schmidt, law; +Cc: gcc-patches
[-- 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. */
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks
2015-10-16 8:03 ` refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks Christian Bruel
@ 2015-10-16 9:47 ` Bernd Schmidt
2015-10-16 10:50 ` Christian Bruel
0 siblings, 1 reply; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-16 9:47 UTC (permalink / raw)
To: Christian Bruel, law; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks
2015-10-16 9:47 ` Bernd Schmidt
@ 2015-10-16 10:50 ` Christian Bruel
2015-10-16 10:56 ` Christian Bruel
2015-10-16 10:56 ` Bernd Schmidt
0 siblings, 2 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-16 10:50 UTC (permalink / raw)
To: Bernd Schmidt, law; +Cc: gcc-patches
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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks
2015-10-16 10:50 ` Christian Bruel
@ 2015-10-16 10:56 ` Christian Bruel
2015-10-16 10:56 ` Bernd Schmidt
1 sibling, 0 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-16 10:56 UTC (permalink / raw)
To: Bernd Schmidt, law; +Cc: gcc-patches
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
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks
2015-10-16 10:50 ` Christian Bruel
2015-10-16 10:56 ` Christian Bruel
@ 2015-10-16 10:56 ` Bernd Schmidt
1 sibling, 0 replies; 28+ messages in thread
From: Bernd Schmidt @ 2015-10-16 10:56 UTC (permalink / raw)
To: Christian Bruel, law; +Cc: gcc-patches
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
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2
2015-10-13 8:03 ` [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Ramana Radhakrishnan
2015-10-13 10:18 ` Christian Bruel
@ 2015-10-16 14:18 ` Christian Bruel
1 sibling, 0 replies; 28+ messages in thread
From: Christian Bruel @ 2015-10-16 14:18 UTC (permalink / raw)
To: Ramana Radhakrishnan; +Cc: Bernd Schmidt, gcc-patches
[-- 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
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2015-10-16 14:12 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-09-29 13:44 [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Christian Bruel
2015-09-29 13:59 ` Christian Bruel
2015-09-30 19:30 ` Jeff Law
2015-10-01 7:12 ` Christian Bruel
2015-10-01 16:11 ` Christian Bruel
2015-10-07 7:05 ` Christian Bruel
2015-10-07 10:18 ` Bernd Schmidt
2015-10-07 10:45 ` Christian Bruel
2015-10-07 17:37 ` Bernd Schmidt
2015-10-07 17:50 ` Bernd Schmidt
2015-10-08 13:14 ` Christian Bruel
2015-10-08 13:20 ` Bernd Schmidt
2015-10-08 13:51 ` Christian Bruel
2015-10-08 14:30 ` Bernd Schmidt
2015-10-12 10:56 ` Christian Bruel
2015-10-12 11:19 ` Bernd Schmidt
2015-10-12 11:26 ` Christian Bruel
2015-10-16 8:03 ` refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks Christian Bruel
2015-10-16 9:47 ` Bernd Schmidt
2015-10-16 10:50 ` Christian Bruel
2015-10-16 10:56 ` Christian Bruel
2015-10-16 10:56 ` Bernd Schmidt
2015-10-13 8:03 ` [PATCH ARM]: PR67745: Fix function alignment after __attribute__ 2/2 Ramana Radhakrishnan
2015-10-13 10:18 ` Christian Bruel
2015-10-16 14:18 ` Christian Bruel
2015-10-08 14:01 ` Christian Bruel
2015-10-08 14:05 ` Bernd Schmidt
2015-10-07 17:40 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).