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

[Tom de Vries <Tom_deVries at mentor dot com>]

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?

Thanks,
- Tom

Add copy for va_list parameter

---
 gcc/function.c | 29 +++++++++++++++++++++++++++++
 gcc/gimplify.c | 16 ----------------
 2 files changed, 29 insertions(+), 16 deletions(-)

diff --git a/gcc/function.c b/gcc/function.c
index 7d4df92..2ebfec4 100644
--- a/gcc/function.c
+++ b/gcc/function.c
@@ -3855,6 +3855,24 @@ gimplify_parm_type (tree *tp, int *walk_subtrees, void *data)
   return NULL;
 }
 
+static inline bool
+is_va_list_type (tree type)
+{
+  tree id = TYPE_IDENTIFIER (type);
+  if (id == NULL_TREE)
+    return false;
+  const char *s = IDENTIFIER_POINTER (id);
+  if (s == NULL)
+    return false;
+  if (strcmp (s, "va_list") == 0)
+    return true;
+  if (strcmp (s, "__builtin_sysv_va_list") == 0)
+    return true;
+  if (strcmp (s, "__builtin_ms_va_list") == 0)
+    return true;
+  return false;
+}
+
 /* Gimplify the parameter list for current_function_decl.  This involves
    evaluating SAVE_EXPRs of variable sized parameters and generating code
    to implement callee-copies reference parameters.  Returns a sequence of
@@ -3953,6 +3971,17 @@ gimplify_parameters (void)
 	      DECL_HAS_VALUE_EXPR_P (parm) = 1;
 	    }
 	}
+      else if (is_va_list_type (TREE_TYPE (parm)))
+	{
+	  tree cp = create_tmp_reg (data.nominal_type, get_name (parm));
+	  DECL_IGNORED_P (cp) = 0;
+	  TREE_ADDRESSABLE (cp) = 1;
+	  tree t = build2 (MODIFY_EXPR, TREE_TYPE (cp), cp, parm);
+	  gimplify_and_add (t, &stmts);
+
+	  SET_DECL_VALUE_EXPR (parm, cp);
+	  DECL_HAS_VALUE_EXPR_P (parm) = 1;
+	}
     }
 
   fnargs.release ();
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 5f1dd1a..c922dc7 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4569,7 +4569,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gimple assign;
   location_t loc = EXPR_LOCATION (*expr_p);
   gimple_stmt_iterator gsi;
-  tree ap = NULL_TREE, ap_copy = NULL_TREE;
 
   gcc_assert (TREE_CODE (*expr_p) == MODIFY_EXPR
 	      || TREE_CODE (*expr_p) == INIT_EXPR);
@@ -4730,16 +4729,12 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	  enum internal_fn ifn = CALL_EXPR_IFN (*from_p);
 	  auto_vec<tree> vargs (nargs);
 
-	  if (ifn == IFN_VA_ARG)
-	    ap = unshare_expr (CALL_EXPR_ARG (*from_p, 0));
 	  for (i = 0; i < nargs; i++)
 	    {
 	      gimplify_arg (&CALL_EXPR_ARG (*from_p, i), pre_p,
 			    EXPR_LOCATION (*from_p));
 	      vargs.quick_push (CALL_EXPR_ARG (*from_p, i));
 	    }
-	  if (ifn == IFN_VA_ARG)
-	    ap_copy = CALL_EXPR_ARG (*from_p, 0);
 	  call_stmt = gimple_build_call_internal_vec (ifn, vargs);
 	  gimple_set_location (call_stmt, EXPR_LOCATION (*expr_p));
 	}
@@ -4784,17 +4779,6 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   gsi = gsi_last (*pre_p);
   maybe_fold_stmt (&gsi);
 
-  /* 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
-      && !operand_equal_p (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), 0))
-    gimplify_assign (TREE_OPERAND (ap, 0), TREE_OPERAND (ap_copy, 0), pre_p);
-
   if (want_value)
     {
       *expr_p = TREE_THIS_VOLATILE (*to_p) ? *from_p : unshare_expr (*to_p);
-- 
1.9.1