Postpone expanding va_arg until pass_stdarg

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

[ was: Re: pass_stdarg problem when run after pass_lim ]

On 03-02-15 14:36, Michael Matz wrote:
> Hi,
>
> On Tue, 3 Feb 2015, Tom de Vries wrote:
>
> > Ironically, that fix breaks the va_list_gpr/fpr_size optimization, so
> > I've disabled that by default for now.
> >
> > I've done a non-bootstrap and bootstrap build using all languages.
> >
> > The non-bootstrap test shows (at least) two classes of real failures:
> > - gcc.c-torture/execute/20020412-1.c, gcc.target/i386/memcpy-strategy-4.c and
> >    gcc.dg/lto/20090706-1_0.c.
> >    These are test-cases with vla as va_arg argument. It ICEs in
> >    force_constant_size with call stack
> >    gimplify_va_arg_expr -> create_tmp_var -> gimple_add_tmp_var ->
> >    force_constant_size
>
> Hah, yeah, that's the issue I remembered with create_tmp_var.  This needs
> a change in how to represent the va_arg "call", because the LHS can't be a
> temporary that's copied to the real LHS afterwards.
>
> > - most/all va_arg tests with flto, f.i. gcc.c-torture/execute/stdarg-1.c.
> >    It segfaults in lto1 during pass_stdarg, in gimplify_va_arg_internal when
> >    accessing have_va_type which is NULL_TREE after
> >    'have_va_type = targetm.canonical_va_list_type (have_va_type)'.
> >
> > I don't think the flto issue is difficult to fix.  But the vla issue
> > probably needs more time than I have available right now.
>
> I'll think about this.

A status update. I've worked a bit more on this patch series, latest version 
available at vries/expand-va-arg-at-pass-stdarg (and last patch in series attached).

I've done a non-bootstrap x86_64 build for all languages and ran the regression 
testsuite for unix/ unix/-m32. I'm left with one failing test-case.
[ Of course there are a bunch of scan-dump-tree failures because the 
va_list_gpr/fpr_size optimization is switched off. ]

The patch series handles things now as follows. At gimplify_va_arg_expr, the 
VA_ARG expr is not gimplified, but replaced by the internal function call.

That is passed upwards to gimplify_modify_expr, which does the actual 
gimplification. In this function we have sufficient scope of the problem to deal 
with it.

I've added two modifications to gimplify_modify_expr:
- the WITH_SIZE_EXPR in which the CALL_TREE is wrapped, is dropped after
  gimplification, but we need the size expression at expansion in pass_stdarg.
  So I added the size expression as argument to the internal function.
  [ And at pass_stdarg::execute, we wrap the result of gimplify_va_arg_internal
  in a WITH_SIZE_EXPR before generating the assign to the lhs ]
- we detect after gimplify_arg (&ap) whether it created a copy ap.1 of ap,
  rather than use ap itself, and if so, we copy the value back from ap.1 to ap
  after va_arg.

I've worked around the issue of targetm.canonical_va_list_type (have_va_type) 
returning NULL_TREE in gimplify_va_arg_internal during lto1, by simply working 
with the original type in that case:
...
  tree have_va_type = TREE_TYPE (valist);
  tree cano_type = targetm.canonical_va_list_type (have_va_type);

  if (cano_type != NULL_TREE)
    have_va_type = cano_type;
...

I'm not really sure yet why std_gimplify_va_arg_expr has a part commented out. 
Michael, can you comment?


The single failing testcase (both with and without -m32) is 
g++.dg/torture/pr45843.C:
...
./gcc/testsuite/g++/g++.sum:FAIL: g++.dg/torture/pr45843.C   -O2 -flto 
-fno-use-linker-plugin -flto-partition=none  (internal compiler error)
...

The failure looks like this (it happens during the gimplify_assign after calling 
gimplify_va_arg_internal):
...
src/gcc/testsuite/g++.dg/torture/pr45843.C: In function ‘foo(int, ...)’:
src/gcc/testsuite/g++.dg/torture/pr45843.C:11:1: internal compiler error: 
Segmentation fault
0x10a5b04 crash_signal
	src/gcc/toplev.c:383
0x6a8985 tree_check(tree_node*, char const*, int, char const*, tree_code)
	src/gcc/tree.h:2845
0x7c2f6a is_really_empty_class(tree_node*)
	src/gcc/cp/class.c:7923
0x923855 cp_gimplify_expr(tree_node**, gimple_statement_base**, 
gimple_statement_base**)
	src/gcc/cp/cp-gimplify.c:625
0xd34641 gimplify_expr(tree_node**, gimple_statement_base**, 
gimple_statement_base**, bool (*)(tree_node*), int)
	src/gcc/gimplify.c:7843
0xd2a04d gimplify_stmt(tree_node**, gimple_statement_base**)
	src/gcc/gimplify.c:5551
0xd173e3 gimplify_and_add(tree_node*, gimple_statement_base**)
	src/gcc/gimplify.c:419
0xd39c94 gimplify_assign(tree_node*, tree_node*, gimple_statement_base**)
	src/gcc/gimplify.c:9452
0x130ad18 execute
	src/gcc/tree-stdarg.c:779
...

The testcase contains this struct:
...
struct S { struct T { } a[14]; char b; };
...

and uses that struct S as type in va_arg:
...
  arg = va_arg (ap, struct S);
...

The segfault happens because we're calling is_really_empty_class for struct S, 
and TYPE_BINFO is NULL_TREE, which causes BINFO_BASE_ITERATE to segfault. I'm 
not sure yet what this issue is or how this is supposed to be fixed.

Thanks,
- Tom

Postpone expanding va_arg until pass_stdarg

---
 gcc/common.opt    |   7 +++-
 gcc/fold-const.c  |   5 +++
 gcc/gimplify.c    | 112 ++++++++++++++++++++++++++++++++++++++++--------------
 gcc/targhooks.c   |   8 ++++
 gcc/tree-cfg.c    |  33 ++++++++++++++++
 gcc/tree-stdarg.c |  91 ++++++++++++++++++++++++++++++++++++++++++--
 6 files changed, 222 insertions(+), 34 deletions(-)

diff --git a/gcc/common.opt b/gcc/common.opt
index 8336337..5489381 100644
--- a/gcc/common.opt
+++ b/gcc/common.opt
@@ -2058,8 +2058,13 @@ fssa-phiopt
 Common Report Var(flag_ssa_phiopt) Optimization
 Optimize conditional patterns using SSA PHI nodes
 
+;; Init this flag to zero, as workaround.  The different way of gimplifying
+;; va_arg breaks this optimization.
+;; TODO: We should be able to remove this workaround, once we reverse the order
+;; in pass_stdarg of expansion of va_arg builtin and and va_list_gpr/fpr_size
+;; optimization.
 ftree-stdarg-opt
-Common Report Var(flag_tree_stdarg_opt) Init(1) Optimization
+Common Report Var(flag_tree_stdarg_opt) Init(0) Optimization
 Optimize amount of stdarg registers saved to stack at start of function
 
 fvariable-expansion-in-unroller
diff --git a/gcc/fold-const.c b/gcc/fold-const.c
index b4301c7..4867979 100644
--- a/gcc/fold-const.c
+++ b/gcc/fold-const.c
@@ -3032,6 +3032,11 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags)
       switch (TREE_CODE (arg0))
 	{
 	case CALL_EXPR:
+	  /* Handle internal_fns conservatively.  */
+	  if (CALL_EXPR_FN (arg0) == NULL_TREE
+	      || CALL_EXPR_FN (arg1) == NULL_TREE)
+	    return 0;
+
 	  /* If the CALL_EXPRs call different functions, then they
 	     clearly can not be equal.  */
 	  if (! operand_equal_p (CALL_EXPR_FN (arg0), CALL_EXPR_FN (arg1),
diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index 1353ada..9b2dbf0 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -4564,6 +4564,7 @@ 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);
@@ -4640,6 +4641,27 @@ gimplify_modify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
   if (ret == GS_ERROR)
     return ret;
 
+  /* In case of va_arg internal fn wrappped in a WITH_SIZE_EXPR, add the type
+     size as argument to the the call.  */
+  if (TREE_CODE (*from_p) == WITH_SIZE_EXPR)
+    {
+      tree call = TREE_OPERAND (*from_p, 0);
+      tree vlasize = TREE_OPERAND (*from_p, 1);
+
+      if (TREE_CODE (call) == CALL_EXPR
+	  && CALL_EXPR_IFN (call) == IFN_VA_ARG)
+	{
+	  tree type = TREE_TYPE (call);
+	  tree ap = CALL_EXPR_ARG (call, 0);
+	  tree tag = CALL_EXPR_ARG (call, 1);
+	  tree newcall = build_call_expr_internal_loc (EXPR_LOCATION (call),
+						       IFN_VA_ARG, type, 3, ap,
+						       tag, vlasize);
+	  tree *call_p = &(TREE_OPERAND (*from_p, 0));
+	  *call_p = newcall;
+	}
+    }
+
   /* Now see if the above changed *from_p to something we handle specially.  */
   ret = gimplify_modify_expr_rhs (expr_p, from_p, to_p, pre_p, post_p,
 				  want_value);
@@ -4703,12 +4725,16 @@ 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));
 	}
@@ -4753,6 +4779,17 @@ 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
+      && TREE_OPERAND (ap, 0) != TREE_OPERAND (ap_copy, 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);
@@ -9290,6 +9327,42 @@ dummy_object (tree type)
   return build2 (MEM_REF, type, t, t);
 }
 
+/* Call the target expander for evaluating a va_arg call of VALIST
+   and TYPE.  */
+
+tree
+gimplify_va_arg_internal (tree valist, tree type, location_t loc,
+			  gimple_seq *pre_p, gimple_seq *post_p)
+{
+  tree have_va_type = TREE_TYPE (valist);
+  tree cano_type = targetm.canonical_va_list_type (have_va_type);
+
+  if (cano_type != NULL_TREE)
+    have_va_type = cano_type;
+
+  /* Make it easier for the backends by protecting the valist argument
+     from multiple evaluations.  */
+  if (TREE_CODE (have_va_type) == ARRAY_TYPE)
+    {
+      /* For this case, the backends will be expecting a pointer to
+	 TREE_TYPE (abi), but it's possible we've
+	 actually been given an array (an actual TARGET_FN_ABI_VA_LIST).
+	 So fix it.  */
+      if (TREE_CODE (TREE_TYPE (valist)) == ARRAY_TYPE)
+	{
+	  tree p1 = build_pointer_type (TREE_TYPE (have_va_type));
+	  valist = fold_convert_loc (loc, p1,
+				     build_fold_addr_expr_loc (loc, valist));
+	}
+
+      gimplify_expr (&valist, pre_p, post_p, is_gimple_val, fb_rvalue);
+    }
+  else
+    gimplify_expr (&valist, pre_p, post_p, is_gimple_min_lval, fb_lvalue);
+
+  return targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p);
+}
+
 /* Gimplify __builtin_va_arg, aka VA_ARG_EXPR, which is not really a
    builtin function, but a very special sort of operator.  */
 
@@ -9316,8 +9389,7 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
 
   /* Generate a diagnostic for requesting data of a type that cannot
      be passed through `...' due to type promotion at the call site.  */
-  if ((promoted_type = lang_hooks.types.type_promotes_to (type))
-	   != type)
+  if ((promoted_type = lang_hooks.types.type_promotes_to (type)) != type)
     {
       static bool gave_help;
       bool warned;
@@ -9351,36 +9423,18 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
       *expr_p = dummy_object (type);
       return GS_ALL_DONE;
     }
-  else
+  else if (optimize && !optimize_debug)
     {
-      /* Make it easier for the backends by protecting the valist argument
-	 from multiple evaluations.  */
-      if (TREE_CODE (have_va_type) == ARRAY_TYPE)
-	{
-	  /* For this case, the backends will be expecting a pointer to
-	     TREE_TYPE (abi), but it's possible we've
-	     actually been given an array (an actual TARGET_FN_ABI_VA_LIST).
-	     So fix it.  */
-	  if (TREE_CODE (TREE_TYPE (valist)) == ARRAY_TYPE)
-	    {
-	      tree p1 = build_pointer_type (TREE_TYPE (have_va_type));
-	      valist = fold_convert_loc (loc, p1,
-					 build_fold_addr_expr_loc (loc, valist));
-	    }
+      tree ap, tag;
 
-	  gimplify_expr (&valist, pre_p, post_p, is_gimple_val, fb_rvalue);
-	}
-      else
-	gimplify_expr (&valist, pre_p, post_p, is_gimple_min_lval, fb_lvalue);
-
-      if (!targetm.gimplify_va_arg_expr)
-	/* FIXME: Once most targets are converted we should merely
-	   assert this is non-null.  */
-	return GS_ALL_DONE;
-
-      *expr_p = targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p);
-      return GS_OK;
+      /* Transform a VA_ARG_EXPR into an VA_ARG internal function.  */
+      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);
     }
+  else
+    *expr_p = gimplify_va_arg_internal (valist, type, loc, pre_p, post_p);
+  return GS_OK;
 }
 
 /* Build a new GIMPLE_ASSIGN tuple and append it to the end of *SEQ_P.
diff --git a/gcc/targhooks.c b/gcc/targhooks.c
index 0c14103..e39a059 100644
--- a/gcc/targhooks.c
+++ b/gcc/targhooks.c
@@ -1817,6 +1817,13 @@ std_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p,
   if (boundary > align
       && !integer_zerop (TYPE_SIZE (type)))
     {
+      t = fold_build_pointer_plus_hwi (valist_tmp, boundary - 1);
+      t = fold_build2 (BIT_AND_EXPR, TREE_TYPE (valist), t,
+		       build_int_cst (TREE_TYPE (valist), -boundary));
+      gimplify_expr (&t, pre_p, post_p, is_gimple_val, fb_rvalue);
+      valist_tmp = t;
+#if 0
+      t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
       t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist_tmp,
 		  fold_build_pointer_plus_hwi (valist_tmp, boundary - 1));
       gimplify_and_add (t, pre_p);
@@ -1826,6 +1833,7 @@ std_gimplify_va_arg_expr (tree valist, tree type, gimple_seq *pre_p,
 			       valist_tmp,
 			       build_int_cst (TREE_TYPE (valist), -boundary)));
       gimplify_and_add (t, pre_p);
+#endif
     }
   else
     boundary = align;
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 90ce455..6550362 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -1033,6 +1033,39 @@ make_edges (void)
   fold_cond_expr_cond ();
 }
 
+/* TODO: Move to tree-cfg.h.  */
+extern bool gimple_find_sub_bbs (gimple_seq, gimple_stmt_iterator *);
+
+bool
+gimple_find_sub_bbs (gimple_seq seq, gimple_stmt_iterator *gsi)
+{
+  gimple stmt = gsi_stmt (*gsi);
+  basic_block bb = gimple_bb (stmt);
+  basic_block lastbb, afterbb;
+  int old_num_bbs = n_basic_blocks_for_fn (cfun);
+  edge e;
+  lastbb = make_blocks_1 (seq, bb);
+  if (old_num_bbs == n_basic_blocks_for_fn (cfun))
+    return false;
+  e = split_block (bb, stmt);
+  /* Move e->dest to come after the new basic blocks.  */
+  afterbb = e->dest;
+  unlink_block (afterbb);
+  link_block (afterbb, lastbb);
+  redirect_edge_succ (e, bb->next_bb);
+  bb = bb->next_bb;
+  while (bb != afterbb)
+    {
+      struct omp_region *cur_region = NULL;
+      int cur_omp_region_idx = 0;
+      int mer = make_edges_bb (bb, &cur_region, &cur_omp_region_idx);
+      gcc_assert (!mer && !cur_region);
+      add_bb_to_loop (bb, afterbb->loop_father);
+      bb = bb->next_bb;
+    }
+  return true;
+}
+
 /* Find the next available discriminator value for LOCUS.  The
    discriminator distinguishes among several basic blocks that
    share a common locus, allowing for more accurate sample-based
diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c
index 3713256..443f6d3 100644
--- a/gcc/tree-stdarg.c
+++ b/gcc/tree-stdarg.c
@@ -52,10 +52,12 @@ along with GCC; see the file COPYING3.  If not see
 #include "gimple-iterator.h"
 #include "gimple-walk.h"
 #include "gimple-ssa.h"
+#include "gimplify.h"
 #include "tree-phinodes.h"
 #include "ssa-iterators.h"
 #include "stringpool.h"
 #include "tree-ssanames.h"
+#include "tree-into-ssa.h"
 #include "sbitmap.h"
 #include "tree-pass.h"
 #include "tree-stdarg.h"
@@ -679,6 +681,16 @@ check_all_va_list_escapes (struct stdarg_info *si)
 }
 
 
+/* TODO: Move to tree-cfg.h.  */
+extern bool gimple_find_sub_bbs (gimple_seq seq, gimple_stmt_iterator *gsi);
+
+/* TODO: move to gimple-iterator.h.  */
+extern void update_modified_stmts (gimple_seq seq);
+
+/* TODO: move to gimplify.h.  */
+extern tree gimplify_va_arg_internal (tree, tree, location_t, gimple_seq *,
+				      gimple_seq *);
+
 namespace {
 
 const pass_data pass_data_stdarg =
@@ -702,11 +714,9 @@ public:
   {}
 
   /* opt_pass methods: */
-  virtual bool gate (function *fun)
+  virtual bool gate (function *)
     {
-      /* This optimization is only for stdarg functions.  */
-      return (flag_tree_stdarg_opt
-	      && fun->stdarg != 0);
+      return true;
     }
 
   virtual unsigned int execute (function *);
@@ -723,6 +733,79 @@ pass_stdarg::execute (function *fun)
   struct walk_stmt_info wi;
   const char *funcname = NULL;
   tree cfun_va_list;
+  unsigned int retflags = 0;
+
+  /* Expand va_arg.  */
+  /* TODO: teach pass_stdarg how process the va_arg builtin, and reverse the
+     order of:
+     - expansion of va_arg builtin, and
+     - va_list_gpr/fpr_size optimization.  */
+  FOR_EACH_BB_FN (bb, fun)
+    {
+      gimple_stmt_iterator i;
+
+      for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i))
+	{
+	  gimple stmt = gsi_stmt (i);
+
+	  if (is_gimple_call (stmt)
+	      && gimple_call_internal_p (stmt)
+	      && gimple_call_internal_fn (stmt) == IFN_VA_ARG)
+	    {
+	      gimple_seq pre = NULL, post = NULL;
+	      tree type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 1)));
+	      tree ap;
+	      tree expr;
+	      tree lhs = gimple_call_lhs (stmt);
+
+	      ap = gimple_call_arg (stmt, 0);
+	      ap = build_fold_indirect_ref (ap);
+
+	      push_gimplify_context (false);
+	      expr = gimplify_va_arg_internal (ap, type, gimple_location (stmt),
+					       &pre, &post);
+
+	      if (lhs != NULL_TREE)
+		{
+		  gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type));
+
+		  /* We've transported the size of with WITH_SIZE_EXPR here as
+		     the 3rd argument of the internal fn call.  Now reinstate
+		     it.  */
+		  if (gimple_call_num_args (stmt) == 3)
+		    expr = build2 (WITH_SIZE_EXPR, TREE_TYPE (expr), expr,
+				   gimple_call_arg (stmt, 2));
+
+		  gimplify_assign (lhs, expr, &pre);
+		}
+
+	      pop_gimplify_context (NULL);
+
+	      gimple_seq_add_seq (&pre, post);
+
+	      update_modified_stmts (pre);
+	      gimple_find_sub_bbs (pre, &i);
+#if 0
+	      gsi_insert_seq_after (&i, pre, GSI_SAME_STMT);
+#endif
+	      gsi_remove (&i, true);
+	      retflags = TODO_update_ssa;
+	      if (gsi_end_p (i))
+		break;
+	    }
+	}
+    }
+
+  if (retflags)
+    {
+      free_dominance_info (CDI_DOMINATORS);
+      update_ssa (retflags);
+    }
+
+  if (!flag_tree_stdarg_opt
+      /* This optimization is only for stdarg functions.  */
+      || fun->stdarg == 0)
+    return 0;
 
   fun->va_list_gpr_size = 0;
   fun->va_list_fpr_size = 0;
-- 
1.9.1