* 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).