This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][PR65823] Fix va_arg ap_copy nop detection


On 22-04-15 15:50, Richard Biener wrote:
On Wed, Apr 22, 2015 at 3:38 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
On 22-04-15 10:06, Richard Biener wrote:

On Wed, Apr 22, 2015 at 9:41 AM, Tom de Vries <Tom_deVries@mentor.com>
wrote:

Hi,

this patch fixes PR65823.


<SNIP>


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.


Committed.

But I wonder if we can't fix things to not require that
odd extra copy.


Agreed, that would be good.

In fact that we introduce ap.1 looks completely bogus to me


AFAICT, it's introduced by gimplify_arg ('&argp') because argp (a PARM_DECL)
is not addressable.

(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"?


[ Originally mentioned at https://gcc.gnu.org/ml/gcc/2015-02/msg00011.html .
]

I've committed gcc.target/x86_64/abi/callabi/vaarg-6.c specifically as a
minimal version of this problem.

If we remove the ap_copy fixup, at original we have:
...
;; Function do_cpy2 (null)
{
   char * e;

     char * e;
   e = VA_ARG_EXPR <argp>;
   e = VA_ARG_EXPR <argp>;
   if (e != b)
     {
       abort ();
     }
}
...

and after gimplify we have:
...
do_cpy2 (char * argp)
{
   char * argp.1;
   char * argp.2;
   char * b.3;
   char * e;

   argp.1 = argp;
   e = VA_ARG (&argp.1, 0B);
   argp.2 = argp;
   e = VA_ARG (&argp.2, 0B);
   b.3 = b;
   if (e != b.3) goto <D.1373>; else goto <D.1374>;
   <D.1373>:
   abort ();
   <D.1374>:
}
...

The second VA_ARG uses &argp.2, which is a copy of argp, which is unmodified
by the first VA_ARG.


Using attached _proof-of-concept_ patch, I get callabi.exp working without
the ap_copy, still at the cost of one 'argp.1 = argp' copy though:
...
do_cpy2 (char * argp)
{
   char * argp.1;
   char * b.2;
   char * e;

   argp.1 = argp;
   e = VA_ARG (&argp.1, 0B);
   e = VA_ARG (&argp.1, 0B);
   b.2 = b;
   if (e != b.2) goto <D.1372>; else goto <D.1373>;
   <D.1372>:
   abort ();
   <D.1373>:
}
...

But perhaps there's an easier way?

Hum, simply

Index: gcc/gimplify.c
===================================================================
--- gcc/gimplify.c      (revision 222320)
+++ gcc/gimplify.c      (working copy)
@@ -9419,6 +9419,7 @@ gimplify_va_arg_expr (tree *expr_p, gimp
      }

    /* Transform a VA_ARG_EXPR into an VA_ARG internal function.  */
+  mark_addressable (valist);
    ap = build_fold_addr_expr_loc (loc, valist);
    tag = build_int_cst (build_pointer_type (type), 0);
    *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap, tag);


That sort of works, but causes other problems. Filed PR65887 - 'remove va_arg ap copies' to track this issue.

Thanks,
- Tom


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]