[ 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