Re: PR 49169: testing the alignment of a function

[Richard Guenther <richard dot guenther at gmail dot com>]

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>
>
>