From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
To: Richard Biener <rguenther@suse.de>
Cc: Richard Biener <richard.guenther@gmail.com>,
Alan Lawrence <Alan.Lawrence@arm.com>,
"gcc-patches@gcc.gnu.org" <gcc-patches@gcc.gnu.org>,
Marcus Shawcroft <Marcus.Shawcroft@arm.com>
Subject: Re: New regression on ARM Linux
Date: Tue, 31 Mar 2015 10:40:00 -0000 [thread overview]
Message-ID: <551A7981.30805@foss.arm.com> (raw)
In-Reply-To: <alpine.LSU.2.11.1503311229310.31545@zhemvz.fhfr.qr>
On 31/03/15 11:36, Richard Biener wrote:
> On Tue, 31 Mar 2015, Richard Earnshaw wrote:
>
>> On 31/03/15 11:00, Richard Biener wrote:
>>> On Tue, 31 Mar 2015, Richard Earnshaw wrote:
>>>
>>>> On 31/03/15 08:50, Richard Biener wrote:
>>>>> On Mon, Mar 30, 2015 at 10:13 PM, Richard Biener <rguenther@suse.de> wrote:
>>>>>> On March 30, 2015 6:45:34 PM GMT+02:00, Alan Lawrence <alan.lawrence@arm.com> wrote:
>>>>>>> -O2 was what I first used; it also occurs at -O1. -fno-tree-sra fixes
>>>>>>> it.
>>>>>>>
>>>>>>> The problem appears to be in laying out arguments, specifically
>>>>>>> varargs. From
>>>>>>> the "good" -fdump-rtl-expand:
>>>>>>>
>>>>>>> (insn 18 17 19 2 (set (mem:SI (reg/f:SI 107 virtual-outgoing-args) [0
>>>>>>> S4 A32])
>>>>>>> (reg:SI 111 [ b1$16 ])) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 19 18 20 2 (set (reg:DF 2 r2)
>>>>>>> (reg:DF 112 [ b1$8 ])) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 20 19 21 2 (set (reg:SI 1 r1)
>>>>>>> (reg:SI 113 [ b1 ])) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 21 20 22 2 (set (reg:SI 0 r0)
>>>>>>> (reg:SI 118)) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (call_insn 22 21 23 2 (parallel [
>>>>>>> (set (reg:SI 0 r0)
>>>>>>> (call (mem:SI (symbol_ref:SI ("printf") [flags 0x41]
>>>>>>> <function_decl 0x2ab50e856d00 __builtin_printf>) [0 __builtin_printf S4
>>>>>>> A32])
>>>>>>>
>>>>>>> The struct members are
>>>>>>> reg:SI 113 => int a;
>>>>>>> reg:DF 112 => double b;
>>>>>>> reg:SI 111 => int c;
>>>>>>>
>>>>>>> r0 gets the format string; r1 gets int a; r2+r3 get double b; int c is
>>>>>>> pushed
>>>>>>> into virtual-outgoing-args. In contrast, post-change to
>>>>>>> build_ref_of_offset, we get:
>>>>>>>
>>>>>>> (insn 17 16 18 2 (set (reg:SI 118)
>>>>>>> (symbol_ref/v/f:SI ("*.LC1") [flags 0x82] <var_decl 0x2ba57fa8d750
>>>>>>> *.LC1>)) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 18 17 19 2 (set (mem:SI (plus:SI (reg/f:SI 107
>>>>>>> virtual-outgoing-args)
>>>>>>> (const_int 8 [0x8])) [0 S4 A64])
>>>>>>> (reg:SI 111 [ b1$16 ])) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 19 18 20 2 (set (mem:DF (reg/f:SI 107 virtual-outgoing-args) [0
>>>>>>> S8 A64])
>>>>>>> (reg:DF 112 [ b1$8 ])) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 20 19 21 2 (set (reg:SI 2 r2)
>>>>>>> (reg:SI 113 [ b1 ])) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (insn 21 20 22 2 (set (reg:SI 0 r0)
>>>>>>> (reg:SI 118)) reduced.c:14 -1
>>>>>>> (nil))
>>>>>>> (call_insn 22 21 23 2 (parallel [
>>>>>>> (set (reg:SI 0 r0)
>>>>>>> (call (mem:SI (symbol_ref:SI ("printf") [flags 0x41]
>>>>>>> <function_decl 0x2ba57f843d00 __builtin_printf>) [0 __builtin_printf S4
>>>>>>> A32])
>>>>>>>
>>>>>>> r0 still gets the format string, but 'int b1.a' now goes in r2, and the
>>>>>>>
>>>>>>> double+following int are all pushed into virtual-outgoing-args. This is
>>>>>>> because
>>>>>>> arm_function_arg is fed a 64-bit-aligned int as type of the second
>>>>>>> argument (the
>>>>>>> type constructed by build_ref_for_offset); it then executes
>>>>>>> (aapcs_layout_arg,
>>>>>>> arm.c line ~~5914)
>>>>>>>
>>>>>>> /* C3 - For double-word aligned arguments, round the NCRN up to the
>>>>>>> next even number. */
>>>>>>> ncrn = pcum->aapcs_ncrn;
>>>>>>> if ((ncrn & 1) && arm_needs_doubleword_align (mode, type))
>>>>>>> ncrn++;
>>>>>>>
>>>>>>> Which changes r1 to r2. Passing -fno-tree-sra, or removing from the
>>>>>>> testcase
>>>>>>> "*(cls_struct_16byte *)resp = b1", causes arm_function_arg to be fed a
>>>>>>> 32-bit-aligned int instead, which works as previously.
>>>>>>>
>>>>>>> Passing the same members of that struct in a non-vargs call, works ok -
>>>>>>> I think
>>>>>>> because these use the type of the declared parameters, rather than the
>>>>>>> provided
>>>>>>> arguments, and the former do not have the increased alignment from
>>>>>>> build_ref_for_offset.
>>>>>>
>>>>>> It doesn't make sense to use the alignment of passed values. That looks like bs.
>>>>>>
>>>>>> This means that
>>>>>>
>>>>>> Int I __aligned__(8);
>>>>>>
>>>>>> Is passed differently than int.
>>>>>>
>>>>>> Arm_function_arg needs to be fixed.
>>>>>
>>>>> That is,
>>>>>
>>>>> typedef int myint __attribute__((aligned(8)));
>>>>>
>>>>> int main()
>>>>> {
>>>>> myint i = 1;
>>>>> int j = 2;
>>>>> __builtin_printf("%d %d\n", i, j);
>>>>> }
>>>>>
>>>>> or
>>>>>
>>>>> myint i;
>>>>> int j;
>>>>> myint *p = &i;
>>>>> int *q = &j;
>>>>>
>>>>> int main()
>>>>> {
>>>>> __builtin_printf("%d %d", *p, *q);
>>>>> }
>>>>>
>>>>> should behave the same. There isn't a printf modifier for an "aligned int"
>>>>> because that sort of thing doesn't make sense. Special-casing aligned vs.
>>>>> non-aligned values only makes sense for things passed by value on the stack.
>>>>> And then obviously only dependent on the functuion type signature, not
>>>>> on the type of the passed value.
>>>>>
>>>>
>>>> I think the testcase is ill-formed. Just because printf doesn't have
>>>> such a modifier, doesn't mean that another variadic function might not
>>>> have a means to detect when an object in the variadic list needs to be
>>>> over-aligned. As such, the test should really be written as:
>>>
>>> A value doesn't have "alignment". A function may have alignment
>>> requirements on its arguments, clearly printf doesn't.
>>>
>>
>> Values don't. But types do and variadic functions are special in that
>> they derive their types from the types of the actual parameters passed
>> not from the formals in the prototype. Any manipulation of the types
>> should be done in the front end, not in the back end.
>
> The following seems to help the testcase (by luck I'd say?). It
> makes us drop alignment information from the temporaries that
> SRA creates as memory replacement.
>
> But I find it odd that on ARM passing *((aligned_int *)p) as
> vararg (only as varargs?) changes calling conventions independent
> of the functions type signature.
>
> Richard.
>
> Index: gcc/tree-sra.c
> ===================================================================
> --- gcc/tree-sra.c (revision 221770)
> +++ gcc/tree-sra.c (working copy)
> @@ -2012,7 +2012,9 @@ create_access_replacement (struct access
> DECL_CONTEXT (repl) = current_function_decl;
> }
> else
> - repl = create_tmp_var (access->type, "SR");
> + repl = create_tmp_var (build_qualified_type
> + (TYPE_MAIN_VARIANT (access->type),
> + TYPE_QUALS (access->type)), "SR");
> if (TREE_CODE (access->type) == COMPLEX_TYPE
> || TREE_CODE (access->type) == VECTOR_TYPE)
> {
>
I guess we could do something similar (use TYPE_MAIN_VARIANT) in the
backend code - but then we'd always ignore any over-alignment
requirements (or under, for that matter).
R.
next prev parent reply other threads:[~2015-03-31 10:40 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-11 15:09 [PATCH] Fix regression caused by PR65310 fix Richard Biener
2015-03-30 12:15 ` New regression on ARM Linux (was: Re: [PATCH] Fix regression caused by PR65310 fix) Alan Lawrence
2015-03-30 12:18 ` New regression on ARM Linux Alan Lawrence
2015-03-30 13:01 ` Richard Biener
2015-03-30 13:16 ` Richard Biener
2015-03-30 16:45 ` Alan Lawrence
2015-03-30 20:13 ` Richard Biener
2015-03-31 7:50 ` Richard Biener
2015-03-31 9:43 ` Richard Earnshaw
2015-03-31 10:00 ` Richard Biener
2015-03-31 10:10 ` Richard Earnshaw
2015-03-31 10:32 ` Jakub Jelinek
2015-03-31 10:36 ` Richard Biener
2015-03-31 10:40 ` Richard Earnshaw [this message]
2015-03-31 10:45 ` Richard Biener
2015-03-31 10:51 ` Richard Earnshaw
2015-03-31 11:09 ` Richard Biener
2015-03-31 12:15 ` Richard Earnshaw
2015-03-31 12:11 ` Alan Lawrence
2015-03-31 10:47 ` Alan Lawrence
2015-03-31 11:05 ` Richard Biener
2015-03-31 11:07 ` Jakub Jelinek
2015-03-31 11:11 ` Richard Biener
2015-03-31 13:11 ` Alan Lawrence
2015-03-31 13:25 ` Richard Biener
2015-04-02 14:59 ` Alan Lawrence
2015-03-31 10:20 ` Richard Biener
2015-03-31 10:31 ` Richard Earnshaw
2015-03-31 10:45 ` Richard Biener
2015-03-31 10:53 ` Richard Earnshaw
2015-03-31 9:50 ` Alan Lawrence
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=551A7981.30805@foss.arm.com \
--to=richard.earnshaw@foss.arm.com \
--cc=Alan.Lawrence@arm.com \
--cc=Marcus.Shawcroft@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=rguenther@suse.de \
--cc=richard.guenther@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).