* PR 49169: testing the alignment of a function @ 2011-06-24 14:47 Richard Sandiford 2011-06-24 16:58 ` Richard Guenther 0 siblings, 1 reply; 5+ messages in thread From: Richard Sandiford @ 2011-06-24 14:47 UTC (permalink / raw) To: gcc-patches This patch fixes PR 49169, where GCC is incorrectly optimising away a test for whether a function is Thumb rather than ARM. The patch was posted by Richard in the PR: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49169 See the PR for a discussion about whether a target hook is better (or not, IMO). Tested on arm-linux-gnueabi, where it fixes the attached testcase. OK to install? Richard gcc/ 2011-07-24 Richard Guenther <rguenther@suse.de> PR tree-optimization/49169 * fold-const.c (get_pointer_modulus_and_residue): Don't rely on the alignment of function decls. gcc/testsuite/ 2011-07-24 Michael Hope <michael.hope@linaro.org> Richard Sandiford <richard.sandiford@linaro.org> PR tree-optimization/49169 * gcc.dg/torture/pr49169.c: New test. Index: gcc/fold-const.c =================================================================== --- gcc/fold-const.c 2011-06-22 16:48:38.000000000 +0100 +++ gcc/fold-const.c 2011-06-23 17:50:33.000000000 +0100 @@ -9216,7 +9216,8 @@ get_pointer_modulus_and_residue (tree ex *residue = 0; code = TREE_CODE (expr); - if (code == ADDR_EXPR) + if (code == ADDR_EXPR + && TREE_CODE (TREE_OPERAND (expr, 0)) != FUNCTION_DECL) { unsigned int bitalign; bitalign = get_object_alignment_1 (TREE_OPERAND (expr, 0), residue); Index: gcc/testsuite/gcc.dg/torture/pr49169.c =================================================================== --- /dev/null 2011-06-20 08:31:41.268810499 +0100 +++ gcc/testsuite/gcc.dg/torture/pr49169.c 2011-06-23 17:52:24.000000000 +0100 @@ -0,0 +1,13 @@ +#include <stdlib.h> +#include <stdint.h> + +int +main (void) +{ + void *p = main; + if ((intptr_t) p & 1) + abort (); + return 0; +} + +/* { dg-final { scan-assembler "abort" } } */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PR 49169: testing the alignment of a function 2011-06-24 14:47 PR 49169: testing the alignment of a function Richard Sandiford @ 2011-06-24 16:58 ` Richard Guenther 2011-06-27 13:46 ` H.J. Lu 0 siblings, 1 reply; 5+ messages in thread From: Richard Guenther @ 2011-06-24 16:58 UTC (permalink / raw) To: gcc-patches, richard.sandiford On Fri, Jun 24, 2011 at 4:42 PM, Richard Sandiford <richard.sandiford@linaro.org> wrote: > This patch fixes PR 49169, where GCC is incorrectly optimising away > a test for whether a function is Thumb rather than ARM. The patch > was posted by Richard in the PR: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49169 > > See the PR for a discussion about whether a target hook is better > (or not, IMO). > > Tested on arm-linux-gnueabi, where it fixes the attached testcase. > OK to install? Ok. Thanks, Richard. > Richard > > > gcc/ > 2011-07-24 Richard Guenther <rguenther@suse.de> > > PR tree-optimization/49169 > * fold-const.c (get_pointer_modulus_and_residue): Don't rely on > the alignment of function decls. > > gcc/testsuite/ > 2011-07-24 Michael Hope <michael.hope@linaro.org> > Richard Sandiford <richard.sandiford@linaro.org> > > PR tree-optimization/49169 > * gcc.dg/torture/pr49169.c: New test. > > Index: gcc/fold-const.c > =================================================================== > --- gcc/fold-const.c 2011-06-22 16:48:38.000000000 +0100 > +++ gcc/fold-const.c 2011-06-23 17:50:33.000000000 +0100 > @@ -9216,7 +9216,8 @@ get_pointer_modulus_and_residue (tree ex > *residue = 0; > > code = TREE_CODE (expr); > - if (code == ADDR_EXPR) > + if (code == ADDR_EXPR > + && TREE_CODE (TREE_OPERAND (expr, 0)) != FUNCTION_DECL) > { > unsigned int bitalign; > bitalign = get_object_alignment_1 (TREE_OPERAND (expr, 0), residue); > Index: gcc/testsuite/gcc.dg/torture/pr49169.c > =================================================================== > --- /dev/null 2011-06-20 08:31:41.268810499 +0100 > +++ gcc/testsuite/gcc.dg/torture/pr49169.c 2011-06-23 17:52:24.000000000 +0100 > @@ -0,0 +1,13 @@ > +#include <stdlib.h> > +#include <stdint.h> > + > +int > +main (void) > +{ > + void *p = main; > + if ((intptr_t) p & 1) > + abort (); > + return 0; > +} > + > +/* { dg-final { scan-assembler "abort" } } */ > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PR 49169: testing the alignment of a function 2011-06-24 16:58 ` Richard Guenther @ 2011-06-27 13:46 ` H.J. Lu 2011-06-29 8:40 ` Richard Sandiford 0 siblings, 1 reply; 5+ messages in thread From: H.J. Lu @ 2011-06-27 13:46 UTC (permalink / raw) To: Richard Guenther; +Cc: gcc-patches, richard.sandiford On Fri, Jun 24, 2011 at 8:50 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Fri, Jun 24, 2011 at 4:42 PM, Richard Sandiford > <richard.sandiford@linaro.org> wrote: >> This patch fixes PR 49169, where GCC is incorrectly optimising away >> a test for whether a function is Thumb rather than ARM. The patch >> was posted by Richard in the PR: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49169 >> >> See the PR for a discussion about whether a target hook is better >> (or not, IMO). >> >> Tested on arm-linux-gnueabi, where it fixes the attached testcase. >> OK to install? > > Ok. > > Thanks, > Richard. > >> Richard >> >> >> gcc/ >> 2011-07-24 Richard Guenther <rguenther@suse.de> >> >> PR tree-optimization/49169 >> * fold-const.c (get_pointer_modulus_and_residue): Don't rely on >> the alignment of function decls. >> This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49545 -- H.J. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PR 49169: testing the alignment of a function 2011-06-27 13:46 ` H.J. Lu @ 2011-06-29 8:40 ` Richard Sandiford 2011-06-29 9:54 ` Richard Guenther 0 siblings, 1 reply; 5+ messages in thread From: Richard Sandiford @ 2011-06-29 8:40 UTC (permalink / raw) To: H.J. Lu; +Cc: Richard Guenther, gcc-patches "H.J. Lu" <hjl.tools@gmail.com> writes: > This caused: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49545 Sorry for the breakage. I should obviously have tested on x86_64 as well. To recap, there are (at least) two concepts of what "the address of X" can mean. It can mean (a) the address at which X is actually located or (b) the value to which symbol X resolves. The problem is that these two are different for things like Thumb and MIPS16 functions. The DECL_ALIGN on a function should be (a). For example, if we have: void __attribute__((aligned(16))) foo (void) { ... } then foo() really ought to start on a 16-byte boundary, even for Thumb. However, most parts of GCC want (b). In particular, if we want to know the alignment of an ADDR_EXPR, or the alignment of a tree or RTL memory reference, the address is going to come from symbol resolution. As far as I can tell, all uses of get_object_alignment* want (b). In particular, get_object_alignment* isn't used for __alignof__, which might have been one case where (a) made more sense. (Any thoughts on what __alignof__ should mean here? Either way, it's a separate patch.) So DECL_ALIGN gives (a) rather than (b). But as the PR shows, there are cases where we know (and need to know) that (b) >= 2 bytes, namely whenever TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn. This patch uses that to decide whether &function has 1 or 2 bytes of alignment. Note that this isn't just about ARM and MIPS. Some targets define FUNCTION_BOUNDARY in a way that depends on optimisation flags, whereas get_object_alignment previously treated it as an ABI property. It's definitely arguable that those targets are buggy and should be using align_functions instead, but the docs aren't really clear. That's why I'm still not going for a target hook at this stage. It's just too hard to say in general whether a port's current FUNCTION_BOUNDARY is guaranteed by the ABI or not, so I think the default definition of any hook would still be the condition that I've used here. I think it makes sense to leave it like this until someone is motivated to guarantee greater alignment. Tested on x86_64-linux-gnu and arm-linux-gnueabi. OK to install? Richard gcc/ PR tree-optimization/49545 * builtins.c (get_object_alignment_1): Update function comment. Do not use DECL_ALIGN for functions, but test TARGET_PTRMEMFUNC_VBIT_LOCATION instead. * fold-const.c (get_pointer_modulus_and_residue): Don't check for functions here. * tree-ssa-ccp.c (get_value_from_alignment): Likewise. gcc/testsuite/ * gcc.dg/torture/pr49169.c: Restrict to ARM and MIPS targets. Index: gcc/builtins.c =================================================================== --- gcc/builtins.c 2011-06-28 14:06:19.000000000 +0100 +++ gcc/builtins.c 2011-06-29 09:07:00.000000000 +0100 @@ -264,8 +264,15 @@ called_as_built_in (tree node) return is_builtin_name (name); } -/* Return the alignment in bits of EXP, an object. - Don't return more than MAX_ALIGN no matter what. */ +/* Compute values M and N such that M divides (address of EXP - N) and + such that N < M. Store N in *BITPOSP and return M. + + Note that the address (and thus the alignment) computed here is based + on the address to which a symbol resolves, whereas DECL_ALIGN is based + on the address at which an object is actually located. These two + addresses are not always the same. For example, on ARM targets, + the address &foo of a Thumb function foo() has the lowest bit set, + whereas foo() itself starts on an even address. */ unsigned int get_object_alignment_1 (tree exp, unsigned HOST_WIDE_INT *bitposp) @@ -287,7 +294,21 @@ get_object_alignment_1 (tree exp, unsign exp = DECL_INITIAL (exp); if (DECL_P (exp) && TREE_CODE (exp) != LABEL_DECL) - align = DECL_ALIGN (exp); + { + if (TREE_CODE (exp) == FUNCTION_DECL) + { + /* Function addresses can encode extra information besides their + alignment. However, if TARGET_PTRMEMFUNC_VBIT_LOCATION + allows the low bit to be used as a virtual bit, we know + that the address itself must be 2-byte aligned. */ + if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn) + align = 2 * BITS_PER_UNIT; + else + align = BITS_PER_UNIT; + } + else + align = DECL_ALIGN (exp); + } else if (CONSTANT_CLASS_P (exp)) { align = TYPE_ALIGN (TREE_TYPE (exp)); Index: gcc/fold-const.c =================================================================== --- gcc/fold-const.c 2011-06-28 14:06:19.000000000 +0100 +++ gcc/fold-const.c 2011-06-29 08:59:02.000000000 +0100 @@ -9216,8 +9216,7 @@ get_pointer_modulus_and_residue (tree ex *residue = 0; code = TREE_CODE (expr); - if (code == ADDR_EXPR - && TREE_CODE (TREE_OPERAND (expr, 0)) != FUNCTION_DECL) + if (code == ADDR_EXPR) { unsigned int bitalign; bitalign = get_object_alignment_1 (TREE_OPERAND (expr, 0), residue); Index: gcc/tree-ssa-ccp.c =================================================================== --- gcc/tree-ssa-ccp.c 2011-06-28 14:06:19.000000000 +0100 +++ gcc/tree-ssa-ccp.c 2011-06-29 08:59:02.000000000 +0100 @@ -520,10 +520,6 @@ get_value_from_alignment (tree expr) val = bit_value_binop (PLUS_EXPR, TREE_TYPE (expr), TREE_OPERAND (base, 0), TREE_OPERAND (base, 1)); else if (base - /* ??? While function decls have DECL_ALIGN their addresses - may encode extra information in the lower bits on some - targets (PR47239). Simply punt for function decls for now. */ - && TREE_CODE (base) != FUNCTION_DECL && ((align = get_object_alignment (base, BIGGEST_ALIGNMENT)) > BITS_PER_UNIT)) { Index: gcc/testsuite/gcc.dg/torture/pr49169.c =================================================================== --- gcc/testsuite/gcc.dg/torture/pr49169.c 2011-06-29 08:58:51.000000000 +0100 +++ gcc/testsuite/gcc.dg/torture/pr49169.c 2011-06-29 09:00:41.000000000 +0100 @@ -1,3 +1,5 @@ +/* { dg-do compile { target { arm*-*-* || mips*-*-* } } } */ + #include <stdlib.h> #include <stdint.h> ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: PR 49169: testing the alignment of a function 2011-06-29 8:40 ` Richard Sandiford @ 2011-06-29 9:54 ` Richard Guenther 0 siblings, 0 replies; 5+ messages in thread From: Richard Guenther @ 2011-06-29 9:54 UTC (permalink / raw) To: H.J. Lu, Richard Guenther, gcc-patches, rdsandiford On Wed, Jun 29, 2011 at 10:28 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > "H.J. Lu" <hjl.tools@gmail.com> writes: >> This caused: >> >> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49545 > > Sorry for the breakage. I should obviously have tested on x86_64 as well. > > To recap, there are (at least) two concepts of what "the address of X" > can mean. It can mean (a) the address at which X is actually located > or (b) the value to which symbol X resolves. The problem is that these > two are different for things like Thumb and MIPS16 functions. > > The DECL_ALIGN on a function should be (a). For example, if we have: > > void __attribute__((aligned(16))) foo (void) { ... } > > then foo() really ought to start on a 16-byte boundary, even for Thumb. > However, most parts of GCC want (b). In particular, if we want to know > the alignment of an ADDR_EXPR, or the alignment of a tree or RTL memory > reference, the address is going to come from symbol resolution. > > As far as I can tell, all uses of get_object_alignment* want (b). > In particular, get_object_alignment* isn't used for __alignof__, > which might have been one case where (a) made more sense. > (Any thoughts on what __alignof__ should mean here? Either way, > it's a separate patch.) > > So DECL_ALIGN gives (a) rather than (b). But as the PR shows, > there are cases where we know (and need to know) that (b) >= 2 bytes, > namely whenever TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn. > This patch uses that to decide whether &function has 1 or 2 bytes of > alignment. > > Note that this isn't just about ARM and MIPS. Some targets define > FUNCTION_BOUNDARY in a way that depends on optimisation flags, > whereas get_object_alignment previously treated it as an ABI property. > It's definitely arguable that those targets are buggy and should be > using align_functions instead, but the docs aren't really clear. > > That's why I'm still not going for a target hook at this stage. > It's just too hard to say in general whether a port's current > FUNCTION_BOUNDARY is guaranteed by the ABI or not, so I think > the default definition of any hook would still be the condition > that I've used here. I think it makes sense to leave it like > this until someone is motivated to guarantee greater alignment. > > Tested on x86_64-linux-gnu and arm-linux-gnueabi. OK to install? Ok. Thanks, Richard. > Richard > > > gcc/ > PR tree-optimization/49545 > * builtins.c (get_object_alignment_1): Update function comment. > Do not use DECL_ALIGN for functions, but test > TARGET_PTRMEMFUNC_VBIT_LOCATION instead. > * fold-const.c (get_pointer_modulus_and_residue): Don't check > for functions here. > * tree-ssa-ccp.c (get_value_from_alignment): Likewise. > > gcc/testsuite/ > * gcc.dg/torture/pr49169.c: Restrict to ARM and MIPS targets. > > Index: gcc/builtins.c > =================================================================== > --- gcc/builtins.c 2011-06-28 14:06:19.000000000 +0100 > +++ gcc/builtins.c 2011-06-29 09:07:00.000000000 +0100 > @@ -264,8 +264,15 @@ called_as_built_in (tree node) > return is_builtin_name (name); > } > > -/* Return the alignment in bits of EXP, an object. > - Don't return more than MAX_ALIGN no matter what. */ > +/* Compute values M and N such that M divides (address of EXP - N) and > + such that N < M. Store N in *BITPOSP and return M. > + > + Note that the address (and thus the alignment) computed here is based > + on the address to which a symbol resolves, whereas DECL_ALIGN is based > + on the address at which an object is actually located. These two > + addresses are not always the same. For example, on ARM targets, > + the address &foo of a Thumb function foo() has the lowest bit set, > + whereas foo() itself starts on an even address. */ > > unsigned int > get_object_alignment_1 (tree exp, unsigned HOST_WIDE_INT *bitposp) > @@ -287,7 +294,21 @@ get_object_alignment_1 (tree exp, unsign > exp = DECL_INITIAL (exp); > if (DECL_P (exp) > && TREE_CODE (exp) != LABEL_DECL) > - align = DECL_ALIGN (exp); > + { > + if (TREE_CODE (exp) == FUNCTION_DECL) > + { > + /* Function addresses can encode extra information besides their > + alignment. However, if TARGET_PTRMEMFUNC_VBIT_LOCATION > + allows the low bit to be used as a virtual bit, we know > + that the address itself must be 2-byte aligned. */ > + if (TARGET_PTRMEMFUNC_VBIT_LOCATION == ptrmemfunc_vbit_in_pfn) > + align = 2 * BITS_PER_UNIT; > + else > + align = BITS_PER_UNIT; > + } > + else > + align = DECL_ALIGN (exp); > + } > else if (CONSTANT_CLASS_P (exp)) > { > align = TYPE_ALIGN (TREE_TYPE (exp)); > Index: gcc/fold-const.c > =================================================================== > --- gcc/fold-const.c 2011-06-28 14:06:19.000000000 +0100 > +++ gcc/fold-const.c 2011-06-29 08:59:02.000000000 +0100 > @@ -9216,8 +9216,7 @@ get_pointer_modulus_and_residue (tree ex > *residue = 0; > > code = TREE_CODE (expr); > - if (code == ADDR_EXPR > - && TREE_CODE (TREE_OPERAND (expr, 0)) != FUNCTION_DECL) > + if (code == ADDR_EXPR) > { > unsigned int bitalign; > bitalign = get_object_alignment_1 (TREE_OPERAND (expr, 0), residue); > Index: gcc/tree-ssa-ccp.c > =================================================================== > --- gcc/tree-ssa-ccp.c 2011-06-28 14:06:19.000000000 +0100 > +++ gcc/tree-ssa-ccp.c 2011-06-29 08:59:02.000000000 +0100 > @@ -520,10 +520,6 @@ get_value_from_alignment (tree expr) > val = bit_value_binop (PLUS_EXPR, TREE_TYPE (expr), > TREE_OPERAND (base, 0), TREE_OPERAND (base, 1)); > else if (base > - /* ??? While function decls have DECL_ALIGN their addresses > - may encode extra information in the lower bits on some > - targets (PR47239). Simply punt for function decls for now. */ > - && TREE_CODE (base) != FUNCTION_DECL > && ((align = get_object_alignment (base, BIGGEST_ALIGNMENT)) > > BITS_PER_UNIT)) > { > Index: gcc/testsuite/gcc.dg/torture/pr49169.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr49169.c 2011-06-29 08:58:51.000000000 +0100 > +++ gcc/testsuite/gcc.dg/torture/pr49169.c 2011-06-29 09:00:41.000000000 +0100 > @@ -1,3 +1,5 @@ > +/* { dg-do compile { target { arm*-*-* || mips*-*-* } } } */ > + > #include <stdlib.h> > #include <stdint.h> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2011-06-29 9:20 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-06-24 14:47 PR 49169: testing the alignment of a function Richard Sandiford 2011-06-24 16:58 ` Richard Guenther 2011-06-27 13:46 ` H.J. Lu 2011-06-29 8:40 ` Richard Sandiford 2011-06-29 9:54 ` Richard Guenther
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).