This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection
- From: Richard Biener <richard dot guenther at gmail dot com>
- To: Tom de Vries <Tom_deVries at mentor dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Wed, 22 Apr 2015 10:06:00 +0200
- Subject: Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection
- Authentication-results: sourceware.org; auth=none
- References: <553750B2 dot 8010209 at mentor dot com>
On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> Hi,
>
> this patch fixes PR65823.
>
> The problem is a verify_gimple ICE during compilation of
> gcc.c-torture/execute/stdarg-2.c for arm at -O0/-O1:
> ...
> In function 'f3':
> src/gcc/testsuite/gcc.c-torture/execute/stdarg-2.c:61:1:
> error: incorrect sharing of tree nodes
> aps[4]
> # .MEM_5 = VDEF <.MEM_11>
> aps[4] = aps[4];
> ...
>
>
> Before gimplification, f3 looks like this in the original dump:
> ...
> struct va_list aps[10];
>
> struct va_list aps[10];
> __builtin_va_start ((struct &) (struct *) &aps[4], i);
> x = VA_ARG_EXPR <aps[4]>;
> __builtin_va_end ((struct &) (struct *) &aps[4]);
> ...
>
> After gimplification, it looks like:
> ...
> f3 (int i)
> {
> long intD.5 x.0D.4231;
> struct va_listD.4222 apsD.4227[10];
>
> try
> {
> # USE = anything
> # CLB = anything
> __builtin_va_startD.1052 (&apsD.4227[4], 0);
> # USE = anything
> # CLB = anything
> x.0D.4231 = VA_ARG (&apsD.4227[4], 0B);
> apsD.4227[4] = apsD.4227[4];
> xD.4223 = x.0D.4231;
> # USE = anything
> # CLB = anything
> __builtin_va_endD.1051 (&apsD.4227[4]);
> }
> finally
> {
> apsD.4227 = {CLOBBER};
> }
> }
> ...
>
> The nop 'apsD.4227[4] = apsD.4227[4]' introduced during gimplification is
> not meant to be there.
>
>
> There is already a test 'TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))'
> in gimplify_modify_expr to prevent this nop:
> ...
> /* When gimplifying the &ap argument of va_arg, we might end up with
> ap.1 = ap
> va_arg (&ap.1, 0B)
> We need to assign ap.1 back to ap, otherwise va_arg has no effect on
> ap. */
> if (ap != NULL_TREE
> && TREE_CODE (ap) == ADDR_EXPR
> && TREE_CODE (ap_copy) == ADDR_EXPR
> && TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 0))
> gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0),
> pre_p);
> ...
>
> But the test is a pointer equality test, and it fails in this case.
>
> The patches fixes the problem by using operand_equal_p to do the equality
> test.
>
>
> Bootstrapped and reg-tested on x86_64.
>
> Did minimal non-bootstrap build on arm and reg-tested.
>
> OK for trunk?
Hmm, ok for now. But I wonder if we can't fix things to not require that
odd extra copy. In fact that we introduce ap.1 looks completely bogus to me
(and we don't in this case for arm). Note that the pointer compare obviously
fails because we unshare the expression.
So ... what breaks if we simply remove this odd "fixup"?
Thanks,
Richard.
> Thanks,
> - Tom