| Message ID | 5614C412.5080400@st.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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
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
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.
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
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 >
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
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))