* [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 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-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
* 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 ` Bernd Schmidt 2015-10-16 10:56 ` Christian Bruel 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 ` Bernd Schmidt 2015-10-16 10:56 ` Christian Bruel 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: refactoring TARGET_PTRMEMFUNC_VBIT_LOCATION checks 2015-10-16 10:50 ` Christian Bruel 2015-10-16 10:56 ` Bernd Schmidt @ 2015-10-16 10:56 ` Christian Bruel 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: [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
* 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
* 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-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
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 ` Bernd Schmidt 2015-10-16 10:56 ` Christian Bruel 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).