* [stage1] Postpone expanding va_arg until pass_stdarg @ 2015-02-19 10:48 Tom de Vries 2015-02-19 10:51 ` [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data Tom de Vries ` (4 more replies) 0 siblings, 5 replies; 39+ messages in thread From: Tom de Vries @ 2015-02-19 10:48 UTC (permalink / raw) To: Jakub Jelinek, Richard Biener; +Cc: GCC Patches, Michael Matz Hi, I'm posting this patch series for stage1: - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch - 0002-Add-gimple_find_sub_bbs.patch - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch - 0004-Handle-internal_fn-in-operand_equal_p.patch - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch The patch series - based on Michael's initial patch - postpones expanding va_arg until pass_stdarg, which makes pass_stdarg more robust. Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and unix/-m32 testing. I'll post the patches in reply to this email. Thanks, - Tom ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data 2015-02-19 10:48 [stage1] Postpone expanding va_arg until pass_stdarg Tom de Vries @ 2015-02-19 10:51 ` Tom de Vries 2015-02-19 12:41 ` Richard Biener 2015-02-19 10:59 ` [PATCH][2/5] Add gimple_find_sub_bbs Tom de Vries ` (3 subsequent siblings) 4 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-19 10:51 UTC (permalink / raw) To: Jakub Jelinek, Richard Biener; +Cc: GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 1119 bytes --] On 19-02-15 11:29, Tom de Vries wrote: > Hi, > > I'm posting this patch series for stage1: > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > - 0002-Add-gimple_find_sub_bbs.patch > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > - 0004-Handle-internal_fn-in-operand_equal_p.patch > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > The patch series - based on Michael's initial patch - postpones expanding va_arg > until pass_stdarg, which makes pass_stdarg more robust. > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > unix/-m32 testing. > > I'll post the patches in reply to this email. > This patch disables lang_hooks.gimplify_expr in free_lang_data. I ran into a situation ( mentioned here: https://gcc.gnu.org/ml/gcc/2015-02/msg00074.html ) where gimplify_expr was called during pass_stdarg, which called cp_gimplify_expr, which called is_really_empty_class, which tried to access some language data (TYPE_BINFO) that was already freed, which caused a segmentation fault. This patch fixes that. OK for stage1? Thanks, - Tom [-- Attachment #2: 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch --] [-- Type: text/x-patch, Size: 759 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> * tree.c (free_lang_data): Disable lang_hooks.gimplify_expr. --- gcc/tree.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/gcc/tree.c b/gcc/tree.c index 29f70f8..94b7c56 100644 --- a/gcc/tree.c +++ b/gcc/tree.c @@ -5815,6 +5815,8 @@ free_lang_data (void) still be used indirectly via the get_alias_set langhook. */ lang_hooks.dwarf_name = lhd_dwarf_name; lang_hooks.decl_printable_name = gimple_decl_printable_name; + lang_hooks.gimplify_expr = lhd_gimplify_expr; + /* We do not want the default decl_assembler_name implementation, rather if we have fixed everything we want a wrapper around it asserting that all non-local symbols already got their assembler -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data 2015-02-19 10:51 ` [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data Tom de Vries @ 2015-02-19 12:41 ` Richard Biener 0 siblings, 0 replies; 39+ messages in thread From: Richard Biener @ 2015-02-19 12:41 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 11:29, Tom de Vries wrote: > > Hi, > > > > I'm posting this patch series for stage1: > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > - 0002-Add-gimple_find_sub_bbs.patch > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > The patch series - based on Michael's initial patch - postpones expanding > > va_arg > > until pass_stdarg, which makes pass_stdarg more robust. > > > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > > unix/-m32 testing. > > > > I'll post the patches in reply to this email. > > > > This patch disables lang_hooks.gimplify_expr in free_lang_data. > > I ran into a situation ( mentioned here: > https://gcc.gnu.org/ml/gcc/2015-02/msg00074.html ) where gimplify_expr was > called during pass_stdarg, which called cp_gimplify_expr, which called > is_really_empty_class, which tried to access some language data (TYPE_BINFO) > that was already freed, which caused a segmentation fault. This patch fixes > that. > > OK for stage1? Ok. Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 10:48 [stage1] Postpone expanding va_arg until pass_stdarg Tom de Vries 2015-02-19 10:51 ` [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data Tom de Vries @ 2015-02-19 10:59 ` Tom de Vries 2015-02-19 12:41 ` Richard Biener 2015-02-19 11:06 ` [PATCH][3/5] Factor optimize_va_list_gpr_fpr_size out of pass_stdarg::execute Tom de Vries ` (2 subsequent siblings) 4 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-19 10:59 UTC (permalink / raw) To: Jakub Jelinek, Richard Biener; +Cc: GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 891 bytes --] On 19-02-15 11:29, Tom de Vries wrote: > Hi, > > I'm posting this patch series for stage1: > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > - 0002-Add-gimple_find_sub_bbs.patch > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > - 0004-Handle-internal_fn-in-operand_equal_p.patch > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > The patch series - based on Michael's initial patch - postpones expanding va_arg > until pass_stdarg, which makes pass_stdarg more robust. > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > unix/-m32 testing. > > I'll post the patches in reply to this email. This patch adds gimple_find_sub_bbs. The function adds a gimple sequence at a point in a basic block, and takes care to split up the sequence itself into new basic blocks where necessary. OK for stage1? Thanks, - Tom [-- Attachment #2: 0002-Add-gimple_find_sub_bbs.patch --] [-- Type: text/x-patch, Size: 9567 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> Michael Matz <matz@suse.de> * tree-cfg.c (make_blocks_1): Factor out of ... (make_blocks): ... here. (make_edges_bb): Factor out of ... (make_edges): ... here. (gimple_find_sub_bbs): New function. * tree-cfg.h (gimple_find_sub_bbs): Declare. --- gcc/tree-cfg.c | 258 +++++++++++++++++++++++++++++++++++---------------------- gcc/tree-cfg.h | 1 + 2 files changed, 159 insertions(+), 100 deletions(-) diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 006bc08..51fcf9c 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -513,16 +513,15 @@ gimple_call_initialize_ctrl_altering (gimple stmt) } -/* Build a flowgraph for the sequence of stmts SEQ. */ +/* Insert SEQ after BB and build a flowgraph. */ -static void -make_blocks (gimple_seq seq) +static basic_block +make_blocks_1 (gimple_seq seq, basic_block bb) { gimple_stmt_iterator i = gsi_start (seq); gimple stmt = NULL; bool start_new_block = true; bool first_stmt_of_seq = true; - basic_block bb = ENTRY_BLOCK_PTR_FOR_FN (cfun); while (!gsi_end_p (i)) { @@ -579,8 +578,16 @@ make_blocks (gimple_seq seq) gsi_next (&i); first_stmt_of_seq = false; } + return bb; } +/* Build a flowgraph for the sequence of stmts SEQ. */ + +static void +make_blocks (gimple_seq seq) +{ + make_blocks_1 (seq, ENTRY_BLOCK_PTR_FOR_FN (cfun)); +} /* Create and return a new empty basic block after bb AFTER. */ @@ -807,6 +814,112 @@ handle_abnormal_edges (basic_block *dispatcher_bbs, make_edge (*dispatcher, for_bb, EDGE_ABNORMAL); } +/* Creates outgoing edges for BB. Returns 1 when it ends with an + computed goto, returns 2 when it ends with a statement that + might return to this function via an nonlocal goto, otherwise + return 0. Updates *PCUR_REGION with the OMP region this BB is in. */ + +static int +make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index) +{ + gimple last = last_stmt (bb); + bool fallthru = false; + int ret = 0; + + if (!last) + return ret; + + switch (gimple_code (last)) + { + case GIMPLE_GOTO: + if (make_goto_expr_edges (bb)) + ret = 1; + fallthru = false; + break; + case GIMPLE_RETURN: + { + edge e = make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); + e->goto_locus = gimple_location (last); + fallthru = false; + } + break; + case GIMPLE_COND: + make_cond_expr_edges (bb); + fallthru = false; + break; + case GIMPLE_SWITCH: + make_gimple_switch_edges (as_a <gswitch *> (last), bb); + fallthru = false; + break; + case GIMPLE_RESX: + make_eh_edges (last); + fallthru = false; + break; + case GIMPLE_EH_DISPATCH: + fallthru = make_eh_dispatch_edges (as_a <geh_dispatch *> (last)); + break; + + case GIMPLE_CALL: + /* If this function receives a nonlocal goto, then we need to + make edges from this call site to all the nonlocal goto + handlers. */ + if (stmt_can_make_abnormal_goto (last)) + ret = 2; + + /* If this statement has reachable exception handlers, then + create abnormal edges to them. */ + make_eh_edges (last); + + /* BUILTIN_RETURN is really a return statement. */ + if (gimple_call_builtin_p (last, BUILT_IN_RETURN)) + { + make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); + fallthru = false; + } + /* Some calls are known not to return. */ + else + fallthru = !(gimple_call_flags (last) & ECF_NORETURN); + break; + + case GIMPLE_ASSIGN: + /* A GIMPLE_ASSIGN may throw internally and thus be considered + control-altering. */ + if (is_ctrl_altering_stmt (last)) + make_eh_edges (last); + fallthru = true; + break; + + case GIMPLE_ASM: + make_gimple_asm_edges (bb); + fallthru = true; + break; + + CASE_GIMPLE_OMP: + fallthru = make_gimple_omp_edges (bb, pcur_region, pomp_index); + break; + + case GIMPLE_TRANSACTION: + { + tree abort_label + = gimple_transaction_label (as_a <gtransaction *> (last)); + if (abort_label) + make_edge (bb, label_to_block (abort_label), EDGE_TM_ABORT); + fallthru = true; + } + break; + + default: + gcc_assert (!stmt_ends_bb_p (last)); + fallthru = true; + break; + } + + if (fallthru) + make_edge (bb, bb->next_bb, EDGE_FALLTHRU); + + return ret; +} + /* Join all the blocks in the flowgraph. */ static void @@ -828,107 +941,19 @@ make_edges (void) /* Traverse the basic block array placing edges. */ FOR_EACH_BB_FN (bb, cfun) { - gimple last = last_stmt (bb); - bool fallthru; + int mer; if (bb_to_omp_idx) bb_to_omp_idx[bb->index] = cur_omp_region_idx; - if (last) - { - enum gimple_code code = gimple_code (last); - switch (code) - { - case GIMPLE_GOTO: - if (make_goto_expr_edges (bb)) - ab_edge_goto.safe_push (bb); - fallthru = false; - break; - case GIMPLE_RETURN: - { - edge e = make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); - e->goto_locus = gimple_location (last); - fallthru = false; - } - break; - case GIMPLE_COND: - make_cond_expr_edges (bb); - fallthru = false; - break; - case GIMPLE_SWITCH: - make_gimple_switch_edges (as_a <gswitch *> (last), bb); - fallthru = false; - break; - case GIMPLE_RESX: - make_eh_edges (last); - fallthru = false; - break; - case GIMPLE_EH_DISPATCH: - fallthru = make_eh_dispatch_edges (as_a <geh_dispatch *> (last)); - break; - - case GIMPLE_CALL: - /* If this function receives a nonlocal goto, then we need to - make edges from this call site to all the nonlocal goto - handlers. */ - if (stmt_can_make_abnormal_goto (last)) - ab_edge_call.safe_push (bb); - - /* If this statement has reachable exception handlers, then - create abnormal edges to them. */ - make_eh_edges (last); - - /* BUILTIN_RETURN is really a return statement. */ - if (gimple_call_builtin_p (last, BUILT_IN_RETURN)) - { - make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); - fallthru = false; - } - /* Some calls are known not to return. */ - else - fallthru = !(gimple_call_flags (last) & ECF_NORETURN); - break; + mer = make_edges_bb (bb, &cur_region, &cur_omp_region_idx); + if (mer == 1) + ab_edge_goto.safe_push (bb); + else if (mer == 2) + ab_edge_call.safe_push (bb); - case GIMPLE_ASSIGN: - /* A GIMPLE_ASSIGN may throw internally and thus be considered - control-altering. */ - if (is_ctrl_altering_stmt (last)) - make_eh_edges (last); - fallthru = true; - break; - - case GIMPLE_ASM: - make_gimple_asm_edges (bb); - fallthru = true; - break; - - CASE_GIMPLE_OMP: - fallthru = make_gimple_omp_edges (bb, &cur_region, - &cur_omp_region_idx); - if (cur_region && bb_to_omp_idx == NULL) - bb_to_omp_idx = XCNEWVEC (int, n_basic_blocks_for_fn (cfun)); - break; - - case GIMPLE_TRANSACTION: - { - tree abort_label - = gimple_transaction_label (as_a <gtransaction *> (last)); - if (abort_label) - make_edge (bb, label_to_block (abort_label), EDGE_TM_ABORT); - fallthru = true; - } - break; - - default: - gcc_assert (!stmt_ends_bb_p (last)); - fallthru = true; - } - } - else - fallthru = true; - - if (fallthru) - make_edge (bb, bb->next_bb, EDGE_FALLTHRU); + if (cur_region && bb_to_omp_idx == NULL) + bb_to_omp_idx = XCNEWVEC (int, n_basic_blocks_for_fn (cfun)); } /* Computed gotos are hell to deal with, especially if there are @@ -1008,6 +1033,39 @@ make_edges (void) fold_cond_expr_cond (); } +/* Add SEQ after GSI. Start new bb after GSI, and created further bbs as + needed. Returns true if new bbs were created. */ + +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-cfg.h b/gcc/tree-cfg.h index cd28a80..2fc1e88 100644 --- a/gcc/tree-cfg.h +++ b/gcc/tree-cfg.h @@ -103,5 +103,6 @@ extern void extract_true_false_edges_from_block (basic_block, edge *, edge *); extern unsigned int execute_fixup_cfg (void); extern unsigned int split_critical_edges (void); extern basic_block insert_cond_bb (basic_block, gimple, gimple); +extern bool gimple_find_sub_bbs (gimple_seq, gimple_stmt_iterator *); #endif /* _TREE_CFG_H */ -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 10:59 ` [PATCH][2/5] Add gimple_find_sub_bbs Tom de Vries @ 2015-02-19 12:41 ` Richard Biener 2015-02-19 12:51 ` Jakub Jelinek 2015-02-22 12:47 ` Tom de Vries 0 siblings, 2 replies; 39+ messages in thread From: Richard Biener @ 2015-02-19 12:41 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 11:29, Tom de Vries wrote: > > Hi, > > > > I'm posting this patch series for stage1: > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > - 0002-Add-gimple_find_sub_bbs.patch > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > The patch series - based on Michael's initial patch - postpones expanding > > va_arg > > until pass_stdarg, which makes pass_stdarg more robust. > > > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > > unix/-m32 testing. > > > > I'll post the patches in reply to this email. > > This patch adds gimple_find_sub_bbs. The function adds a gimple sequence at a > point in a basic block, and takes care to split up the sequence itself into > new basic blocks where necessary. > > OK for stage1? I hope we can get rid of this again (which needs re-writing of all targets va-arg gimplification hooks... - we are in need of some generic diamond/triangle CFG pattern builders anyway). Ok with a comment on the function that this is transitional and should not be used in new code. Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 12:41 ` Richard Biener @ 2015-02-19 12:51 ` Jakub Jelinek 2015-02-19 13:03 ` Marek Polacek 2015-02-19 13:07 ` Richard Biener 2015-02-22 12:47 ` Tom de Vries 1 sibling, 2 replies; 39+ messages in thread From: Jakub Jelinek @ 2015-02-19 12:51 UTC (permalink / raw) To: Richard Biener; +Cc: Tom de Vries, GCC Patches, Michael Matz On Thu, Feb 19, 2015 at 01:41:05PM +0100, Richard Biener wrote: > I hope we can get rid of this again (which needs re-writing of all > targets va-arg gimplification hooks... - we are in need of some > generic diamond/triangle CFG pattern builders anyway). We already have some in asan.c - create_cond_insert_point - perhaps it isn't best named and could use some changes to be generally usable. Jakub ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 12:51 ` Jakub Jelinek @ 2015-02-19 13:03 ` Marek Polacek 2015-02-19 13:31 ` Richard Biener 2015-02-19 13:07 ` Richard Biener 1 sibling, 1 reply; 39+ messages in thread From: Marek Polacek @ 2015-02-19 13:03 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Richard Biener, Tom de Vries, GCC Patches, Michael Matz On Thu, Feb 19, 2015 at 01:48:58PM +0100, Jakub Jelinek wrote: > On Thu, Feb 19, 2015 at 01:41:05PM +0100, Richard Biener wrote: > > I hope we can get rid of this again (which needs re-writing of all > > targets va-arg gimplification hooks... - we are in need of some > > generic diamond/triangle CFG pattern builders anyway). > > We already have some in asan.c - create_cond_insert_point - perhaps > it isn't best named and could use some changes to be generally usable. Yeah, create_cond_insert_point is used in ubsan.c many times, and there's nothing *san specific in it. Marek ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 13:03 ` Marek Polacek @ 2015-02-19 13:31 ` Richard Biener 0 siblings, 0 replies; 39+ messages in thread From: Richard Biener @ 2015-02-19 13:31 UTC (permalink / raw) To: Marek Polacek; +Cc: Jakub Jelinek, Tom de Vries, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Marek Polacek wrote: > On Thu, Feb 19, 2015 at 01:48:58PM +0100, Jakub Jelinek wrote: > > On Thu, Feb 19, 2015 at 01:41:05PM +0100, Richard Biener wrote: > > > I hope we can get rid of this again (which needs re-writing of all > > > targets va-arg gimplification hooks... - we are in need of some > > > generic diamond/triangle CFG pattern builders anyway). > > > > We already have some in asan.c - create_cond_insert_point - perhaps > > it isn't best named and could use some changes to be generally usable. > > Yeah, create_cond_insert_point is used in ubsan.c many times, and > there's nothing *san specific in it. It's also quite low-level. I'm more looking for sth like val = gimple_build_cond_value (EQ_EXPR, comp_lhs, comp_rhs, true_value, true_gimple_seq, false_value, false_gimple_seq); which then returns a simplified value (in case all is optimizable to a constant for example) or produces if (comp_lhs == comp_rhs) true_gimple_seq; else false_gimple_seq; val = PHI <true_value, false_value> so basically allows you to build the true/false values with gimple_build (or force_gimple_operand for legacy code) and constructs the diamond/triangle and a PHI together with the controlling predicate. Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 12:51 ` Jakub Jelinek 2015-02-19 13:03 ` Marek Polacek @ 2015-02-19 13:07 ` Richard Biener 1 sibling, 0 replies; 39+ messages in thread From: Richard Biener @ 2015-02-19 13:07 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Tom de Vries, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Jakub Jelinek wrote: > On Thu, Feb 19, 2015 at 01:41:05PM +0100, Richard Biener wrote: > > I hope we can get rid of this again (which needs re-writing of all > > targets va-arg gimplification hooks... - we are in need of some > > generic diamond/triangle CFG pattern builders anyway). > > We already have some in asan.c - create_cond_insert_point - perhaps > it isn't best named and could use some changes to be generally usable. Yeah - we should look at individual uses to see what is needed. I guess some are to generate a COND_EXPR with arms that can have side-effects, thus produce a single value in two different ways dependent on a condition. Others are to just guard a code region. Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][2/5] Add gimple_find_sub_bbs 2015-02-19 12:41 ` Richard Biener 2015-02-19 12:51 ` Jakub Jelinek @ 2015-02-22 12:47 ` Tom de Vries 1 sibling, 0 replies; 39+ messages in thread From: Tom de Vries @ 2015-02-22 12:47 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 1351 bytes --] On 19-02-15 13:41, Richard Biener wrote: > On Thu, 19 Feb 2015, Tom de Vries wrote: > >> On 19-02-15 11:29, Tom de Vries wrote: >>> Hi, >>> >>> I'm posting this patch series for stage1: >>> - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch >>> - 0002-Add-gimple_find_sub_bbs.patch >>> - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch >>> - 0004-Handle-internal_fn-in-operand_equal_p.patch >>> - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch >>> >>> The patch series - based on Michael's initial patch - postpones expanding >>> va_arg >>> until pass_stdarg, which makes pass_stdarg more robust. >>> >>> Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and >>> unix/-m32 testing. >>> >>> I'll post the patches in reply to this email. >> >> This patch adds gimple_find_sub_bbs. The function adds a gimple sequence at a >> point in a basic block, and takes care to split up the sequence itself into >> new basic blocks where necessary. >> >> OK for stage1? > > I hope we can get rid of this again (which needs re-writing of all > targets va-arg gimplification hooks... - we are in need of some > generic diamond/triangle CFG pattern builders anyway). > > Ok with a comment on the function that this is transitional and > should not be used in new code. > Updated with comment. Thanks, - Tom [-- Attachment #2: 0002-Add-gimple_find_sub_bbs.patch --] [-- Type: text/x-patch, Size: 9862 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> Michael Matz <matz@suse.de> * tree-cfg.c (make_blocks_1): Factor out of ... (make_blocks): ... here. (make_edges_bb): Factor out of ... (make_edges): ... here. (gimple_find_sub_bbs): New function. * tree-cfg.h (gimple_find_sub_bbs): Declare. --- gcc/tree-cfg.c | 262 +++++++++++++++++++++++++++++++++++---------------------- gcc/tree-cfg.h | 1 + 2 files changed, 163 insertions(+), 100 deletions(-) diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c index 006bc08..3aaa0b9 100644 --- a/gcc/tree-cfg.c +++ b/gcc/tree-cfg.c @@ -513,16 +513,15 @@ gimple_call_initialize_ctrl_altering (gimple stmt) } -/* Build a flowgraph for the sequence of stmts SEQ. */ +/* Insert SEQ after BB and build a flowgraph. */ -static void -make_blocks (gimple_seq seq) +static basic_block +make_blocks_1 (gimple_seq seq, basic_block bb) { gimple_stmt_iterator i = gsi_start (seq); gimple stmt = NULL; bool start_new_block = true; bool first_stmt_of_seq = true; - basic_block bb = ENTRY_BLOCK_PTR_FOR_FN (cfun); while (!gsi_end_p (i)) { @@ -579,8 +578,16 @@ make_blocks (gimple_seq seq) gsi_next (&i); first_stmt_of_seq = false; } + return bb; } +/* Build a flowgraph for the sequence of stmts SEQ. */ + +static void +make_blocks (gimple_seq seq) +{ + make_blocks_1 (seq, ENTRY_BLOCK_PTR_FOR_FN (cfun)); +} /* Create and return a new empty basic block after bb AFTER. */ @@ -807,6 +814,112 @@ handle_abnormal_edges (basic_block *dispatcher_bbs, make_edge (*dispatcher, for_bb, EDGE_ABNORMAL); } +/* Creates outgoing edges for BB. Returns 1 when it ends with an + computed goto, returns 2 when it ends with a statement that + might return to this function via an nonlocal goto, otherwise + return 0. Updates *PCUR_REGION with the OMP region this BB is in. */ + +static int +make_edges_bb (basic_block bb, struct omp_region **pcur_region, int *pomp_index) +{ + gimple last = last_stmt (bb); + bool fallthru = false; + int ret = 0; + + if (!last) + return ret; + + switch (gimple_code (last)) + { + case GIMPLE_GOTO: + if (make_goto_expr_edges (bb)) + ret = 1; + fallthru = false; + break; + case GIMPLE_RETURN: + { + edge e = make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); + e->goto_locus = gimple_location (last); + fallthru = false; + } + break; + case GIMPLE_COND: + make_cond_expr_edges (bb); + fallthru = false; + break; + case GIMPLE_SWITCH: + make_gimple_switch_edges (as_a <gswitch *> (last), bb); + fallthru = false; + break; + case GIMPLE_RESX: + make_eh_edges (last); + fallthru = false; + break; + case GIMPLE_EH_DISPATCH: + fallthru = make_eh_dispatch_edges (as_a <geh_dispatch *> (last)); + break; + + case GIMPLE_CALL: + /* If this function receives a nonlocal goto, then we need to + make edges from this call site to all the nonlocal goto + handlers. */ + if (stmt_can_make_abnormal_goto (last)) + ret = 2; + + /* If this statement has reachable exception handlers, then + create abnormal edges to them. */ + make_eh_edges (last); + + /* BUILTIN_RETURN is really a return statement. */ + if (gimple_call_builtin_p (last, BUILT_IN_RETURN)) + { + make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); + fallthru = false; + } + /* Some calls are known not to return. */ + else + fallthru = !(gimple_call_flags (last) & ECF_NORETURN); + break; + + case GIMPLE_ASSIGN: + /* A GIMPLE_ASSIGN may throw internally and thus be considered + control-altering. */ + if (is_ctrl_altering_stmt (last)) + make_eh_edges (last); + fallthru = true; + break; + + case GIMPLE_ASM: + make_gimple_asm_edges (bb); + fallthru = true; + break; + + CASE_GIMPLE_OMP: + fallthru = make_gimple_omp_edges (bb, pcur_region, pomp_index); + break; + + case GIMPLE_TRANSACTION: + { + tree abort_label + = gimple_transaction_label (as_a <gtransaction *> (last)); + if (abort_label) + make_edge (bb, label_to_block (abort_label), EDGE_TM_ABORT); + fallthru = true; + } + break; + + default: + gcc_assert (!stmt_ends_bb_p (last)); + fallthru = true; + break; + } + + if (fallthru) + make_edge (bb, bb->next_bb, EDGE_FALLTHRU); + + return ret; +} + /* Join all the blocks in the flowgraph. */ static void @@ -828,107 +941,19 @@ make_edges (void) /* Traverse the basic block array placing edges. */ FOR_EACH_BB_FN (bb, cfun) { - gimple last = last_stmt (bb); - bool fallthru; + int mer; if (bb_to_omp_idx) bb_to_omp_idx[bb->index] = cur_omp_region_idx; - if (last) - { - enum gimple_code code = gimple_code (last); - switch (code) - { - case GIMPLE_GOTO: - if (make_goto_expr_edges (bb)) - ab_edge_goto.safe_push (bb); - fallthru = false; - break; - case GIMPLE_RETURN: - { - edge e = make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); - e->goto_locus = gimple_location (last); - fallthru = false; - } - break; - case GIMPLE_COND: - make_cond_expr_edges (bb); - fallthru = false; - break; - case GIMPLE_SWITCH: - make_gimple_switch_edges (as_a <gswitch *> (last), bb); - fallthru = false; - break; - case GIMPLE_RESX: - make_eh_edges (last); - fallthru = false; - break; - case GIMPLE_EH_DISPATCH: - fallthru = make_eh_dispatch_edges (as_a <geh_dispatch *> (last)); - break; - - case GIMPLE_CALL: - /* If this function receives a nonlocal goto, then we need to - make edges from this call site to all the nonlocal goto - handlers. */ - if (stmt_can_make_abnormal_goto (last)) - ab_edge_call.safe_push (bb); - - /* If this statement has reachable exception handlers, then - create abnormal edges to them. */ - make_eh_edges (last); - - /* BUILTIN_RETURN is really a return statement. */ - if (gimple_call_builtin_p (last, BUILT_IN_RETURN)) - { - make_edge (bb, EXIT_BLOCK_PTR_FOR_FN (cfun), 0); - fallthru = false; - } - /* Some calls are known not to return. */ - else - fallthru = !(gimple_call_flags (last) & ECF_NORETURN); - break; + mer = make_edges_bb (bb, &cur_region, &cur_omp_region_idx); + if (mer == 1) + ab_edge_goto.safe_push (bb); + else if (mer == 2) + ab_edge_call.safe_push (bb); - case GIMPLE_ASSIGN: - /* A GIMPLE_ASSIGN may throw internally and thus be considered - control-altering. */ - if (is_ctrl_altering_stmt (last)) - make_eh_edges (last); - fallthru = true; - break; - - case GIMPLE_ASM: - make_gimple_asm_edges (bb); - fallthru = true; - break; - - CASE_GIMPLE_OMP: - fallthru = make_gimple_omp_edges (bb, &cur_region, - &cur_omp_region_idx); - if (cur_region && bb_to_omp_idx == NULL) - bb_to_omp_idx = XCNEWVEC (int, n_basic_blocks_for_fn (cfun)); - break; - - case GIMPLE_TRANSACTION: - { - tree abort_label - = gimple_transaction_label (as_a <gtransaction *> (last)); - if (abort_label) - make_edge (bb, label_to_block (abort_label), EDGE_TM_ABORT); - fallthru = true; - } - break; - - default: - gcc_assert (!stmt_ends_bb_p (last)); - fallthru = true; - } - } - else - fallthru = true; - - if (fallthru) - make_edge (bb, bb->next_bb, EDGE_FALLTHRU); + if (cur_region && bb_to_omp_idx == NULL) + bb_to_omp_idx = XCNEWVEC (int, n_basic_blocks_for_fn (cfun)); } /* Computed gotos are hell to deal with, especially if there are @@ -1008,6 +1033,43 @@ make_edges (void) fold_cond_expr_cond (); } +/* Add SEQ after GSI. Start new bb after GSI, and created further bbs as + needed. Returns true if new bbs were created. + Note: This is transitional code, and should not be used for new code. We + should be able to get rid of this by rewriting all target va-arg + gimplification hooks to use an interface gimple_build_cond_value as described + in https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01194.html. */ + +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-cfg.h b/gcc/tree-cfg.h index cd28a80..2fc1e88 100644 --- a/gcc/tree-cfg.h +++ b/gcc/tree-cfg.h @@ -103,5 +103,6 @@ extern void extract_true_false_edges_from_block (basic_block, edge *, edge *); extern unsigned int execute_fixup_cfg (void); extern unsigned int split_critical_edges (void); extern basic_block insert_cond_bb (basic_block, gimple, gimple); +extern bool gimple_find_sub_bbs (gimple_seq, gimple_stmt_iterator *); #endif /* _TREE_CFG_H */ -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH][3/5] Factor optimize_va_list_gpr_fpr_size out of pass_stdarg::execute 2015-02-19 10:48 [stage1] Postpone expanding va_arg until pass_stdarg Tom de Vries 2015-02-19 10:51 ` [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data Tom de Vries 2015-02-19 10:59 ` [PATCH][2/5] Add gimple_find_sub_bbs Tom de Vries @ 2015-02-19 11:06 ` Tom de Vries 2015-02-19 12:44 ` Richard Biener 2015-02-19 11:37 ` [PATCH][4/5] Handle internal_fn in operand_equal_p Tom de Vries 2015-02-19 12:08 ` [PATCH][5/5] Postpone expanding va_arg until pass_stdarg Tom de Vries 4 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-19 11:06 UTC (permalink / raw) To: Jakub Jelinek, Richard Biener; +Cc: GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 911 bytes --] On 19-02-15 11:29, Tom de Vries wrote: > Hi, > > I'm posting this patch series for stage1: > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > - 0002-Add-gimple_find_sub_bbs.patch > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > - 0004-Handle-internal_fn-in-operand_equal_p.patch > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > The patch series - based on Michael's initial patch - postpones expanding va_arg > until pass_stdarg, which makes pass_stdarg more robust. > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > unix/-m32 testing. > > I'll post the patches in reply to this email. > This patch factors optimize_va_list_gpr_fpr_size out of pass_stdarg::execute. Now that we're adding more functionality in pass_stdarg, it's cleaner and clearer to move the optimization to its own function. OK for stage1? Thanks, - Tom [-- Attachment #2: 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch --] [-- Type: text/x-patch, Size: 2569 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> * tree-stdarg.c (optimize_va_list_gpr_fpr_size): Factor out of ... (pass_stdarg::execute): ... here. --- gcc/tree-stdarg.c | 80 ++++++++++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 36 deletions(-) diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c index 17d51a2..5eac56a 100644 --- a/gcc/tree-stdarg.c +++ b/gcc/tree-stdarg.c @@ -678,43 +678,10 @@ check_all_va_list_escapes (struct stdarg_info *si) return false; } +/* Optimize FUN->va_list_gpr_size and FUN->va_list_fpr_size. */ -namespace { - -const pass_data pass_data_stdarg = -{ - GIMPLE_PASS, /* type */ - "stdarg", /* name */ - OPTGROUP_NONE, /* optinfo_flags */ - TV_NONE, /* tv_id */ - ( PROP_cfg | PROP_ssa ), /* properties_required */ - 0, /* properties_provided */ - 0, /* properties_destroyed */ - 0, /* todo_flags_start */ - 0, /* todo_flags_finish */ -}; - -class pass_stdarg : public gimple_opt_pass -{ -public: - pass_stdarg (gcc::context *ctxt) - : gimple_opt_pass (pass_data_stdarg, ctxt) - {} - - /* opt_pass methods: */ - virtual bool gate (function *fun) - { - return (flag_stdarg_opt - /* This optimization is only for stdarg functions. */ - && fun->stdarg != 0); - } - - virtual unsigned int execute (function *); - -}; // class pass_stdarg - -unsigned int -pass_stdarg::execute (function *fun) +static void +optimize_va_list_gpr_fpr_size (function *fun) { basic_block bb; bool va_list_escapes = false; @@ -1047,6 +1014,47 @@ finish: fprintf (dump_file, "%d", cfun->va_list_fpr_size); fputs (" FPR units.\n", dump_file); } +} + +namespace { + +const pass_data pass_data_stdarg = +{ + GIMPLE_PASS, /* type */ + "stdarg", /* name */ + OPTGROUP_NONE, /* optinfo_flags */ + TV_NONE, /* tv_id */ + ( PROP_cfg | PROP_ssa ), /* properties_required */ + 0, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ + 0, /* todo_flags_finish */ +}; + +class pass_stdarg : public gimple_opt_pass +{ +public: + pass_stdarg (gcc::context *ctxt) + : gimple_opt_pass (pass_data_stdarg, ctxt) + {} + + /* opt_pass methods: */ + virtual bool gate (function *fun) + { + return (flag_stdarg_opt + /* This optimization is only for stdarg functions. */ + && fun->stdarg != 0); + } + + virtual unsigned int execute (function *); + +}; // class pass_stdarg + +unsigned int +pass_stdarg::execute (function *fun) +{ + optimize_va_list_gpr_fpr_size (fun); + return 0; } -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][3/5] Factor optimize_va_list_gpr_fpr_size out of pass_stdarg::execute 2015-02-19 11:06 ` [PATCH][3/5] Factor optimize_va_list_gpr_fpr_size out of pass_stdarg::execute Tom de Vries @ 2015-02-19 12:44 ` Richard Biener 0 siblings, 0 replies; 39+ messages in thread From: Richard Biener @ 2015-02-19 12:44 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 11:29, Tom de Vries wrote: > > Hi, > > > > I'm posting this patch series for stage1: > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > - 0002-Add-gimple_find_sub_bbs.patch > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > The patch series - based on Michael's initial patch - postpones expanding > > va_arg > > until pass_stdarg, which makes pass_stdarg more robust. > > > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > > unix/-m32 testing. > > > > I'll post the patches in reply to this email. > > > > This patch factors optimize_va_list_gpr_fpr_size out of pass_stdarg::execute. > > Now that we're adding more functionality in pass_stdarg, it's cleaner and > clearer to move the optimization to its own function. > > OK for stage1? Ok. Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-19 10:48 [stage1] Postpone expanding va_arg until pass_stdarg Tom de Vries ` (2 preceding siblings ...) 2015-02-19 11:06 ` [PATCH][3/5] Factor optimize_va_list_gpr_fpr_size out of pass_stdarg::execute Tom de Vries @ 2015-02-19 11:37 ` Tom de Vries 2015-02-19 12:49 ` Richard Biener 2015-02-19 12:08 ` [PATCH][5/5] Postpone expanding va_arg until pass_stdarg Tom de Vries 4 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-19 11:37 UTC (permalink / raw) To: Jakub Jelinek, Richard Biener; +Cc: GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 1333 bytes --] On 19-02-15 11:29, Tom de Vries wrote: > Hi, > > I'm posting this patch series for stage1: > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > - 0002-Add-gimple_find_sub_bbs.patch > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > - 0004-Handle-internal_fn-in-operand_equal_p.patch > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > The patch series - based on Michael's initial patch - postpones expanding va_arg > until pass_stdarg, which makes pass_stdarg more robust. > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > unix/-m32 testing. > > I'll post the patches in reply to this email. > This patch adds handling of internal functions in operand_equal_p. I ran into a situation here in operand_equal_p where it segfaulted on the internal function IFN_VA_ARG, because the CALL_EXPR_FN of an internal function is NULL, and operand_equal_p does not expect NULL arguments: ... case CALL_EXPR: /* 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), flags)) return 0; ... This patch fixes that by testing if CALL_EXPR_FN is NULL. OK for stage1? Thanks, - Tom [-- Attachment #2: 0004-Handle-internal_fn-in-operand_equal_p.patch --] [-- Type: text/x-patch, Size: 775 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> * fold-const.c (operand_equal_p): Handle INTERNAL_FNs. --- gcc/fold-const.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 8377120..fbf76d0 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), -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-19 11:37 ` [PATCH][4/5] Handle internal_fn in operand_equal_p Tom de Vries @ 2015-02-19 12:49 ` Richard Biener 2015-02-19 12:59 ` Jakub Jelinek 0 siblings, 1 reply; 39+ messages in thread From: Richard Biener @ 2015-02-19 12:49 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 11:29, Tom de Vries wrote: > > Hi, > > > > I'm posting this patch series for stage1: > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > - 0002-Add-gimple_find_sub_bbs.patch > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > The patch series - based on Michael's initial patch - postpones expanding > > va_arg > > until pass_stdarg, which makes pass_stdarg more robust. > > > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > > unix/-m32 testing. > > > > I'll post the patches in reply to this email. > > > > This patch adds handling of internal functions in operand_equal_p. > > I ran into a situation here in operand_equal_p where it segfaulted on the > internal function IFN_VA_ARG, because the CALL_EXPR_FN of an internal > function is NULL, and operand_equal_p does not expect NULL arguments: > ... > case CALL_EXPR: > /* 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), > flags)) > return 0; > ... > > This patch fixes that by testing if CALL_EXPR_FN is NULL. > > OK for stage1? I'd call it a bug though, and we do have internal fns in generic already thus the issue is latent (with ubsan at least). Which means ok for trunk now. Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-19 12:49 ` Richard Biener @ 2015-02-19 12:59 ` Jakub Jelinek 2015-02-19 13:09 ` Richard Biener 0 siblings, 1 reply; 39+ messages in thread From: Jakub Jelinek @ 2015-02-19 12:59 UTC (permalink / raw) To: Richard Biener; +Cc: Tom de Vries, GCC Patches, Michael Matz On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > I'd call it a bug though, and we do have internal fns in > generic already thus the issue is latent (with ubsan at least). > > Which means ok for trunk now. But the patch should better handle the internal calls right. I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, or if both are NULL and CALL_EXPR_IFN is different, or if call_expr_nargs is different. Jakub ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-19 12:59 ` Jakub Jelinek @ 2015-02-19 13:09 ` Richard Biener 2015-02-19 15:31 ` Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Richard Biener @ 2015-02-19 13:09 UTC (permalink / raw) To: Jakub Jelinek; +Cc: Tom de Vries, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Jakub Jelinek wrote: > On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > > I'd call it a bug though, and we do have internal fns in > > generic already thus the issue is latent (with ubsan at least). > > > > Which means ok for trunk now. > > But the patch should better handle the internal calls right. > I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, > or if both are NULL and CALL_EXPR_IFN is different, or if > call_expr_nargs is different. The question is whether generic call handling works (esp. call_expr_flags works correctly - the argument compare should work fine already). Tom - care to update the patch? Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-19 13:09 ` Richard Biener @ 2015-02-19 15:31 ` Tom de Vries 2015-02-20 11:59 ` Richard Biener 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-19 15:31 UTC (permalink / raw) To: Richard Biener, Jakub Jelinek; +Cc: GCC Patches, Michael Matz On 19-02-15 14:07, Richard Biener wrote: > On Thu, 19 Feb 2015, Jakub Jelinek wrote: > >> On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: >>> I'd call it a bug though, and we do have internal fns in >>> generic already thus the issue is latent (with ubsan at least). >>> >>> Which means ok for trunk now. >> >> But the patch should better handle the internal calls right. >> I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, >> or if both are NULL and CALL_EXPR_IFN is different, or if >> call_expr_nargs is different. > > The question is whether generic call handling works (esp. call_expr_flags > works correctly - the argument compare should work fine already). > > Tom - care to update the patch? > I agree, the current patch is conservative and we can do better. But I think it's wiser to do that as a stage1 follow-up, and commit this conservative patch for stage4. Is that acceptable? Thanks, - Tom ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-19 15:31 ` Tom de Vries @ 2015-02-20 11:59 ` Richard Biener 2015-02-22 13:13 ` Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Richard Biener @ 2015-02-20 11:59 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 14:07, Richard Biener wrote: > > On Thu, 19 Feb 2015, Jakub Jelinek wrote: > > > > > On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > > > > I'd call it a bug though, and we do have internal fns in > > > > generic already thus the issue is latent (with ubsan at least). > > > > > > > > Which means ok for trunk now. > > > > > > But the patch should better handle the internal calls right. > > > I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, > > > or if both are NULL and CALL_EXPR_IFN is different, or if > > > call_expr_nargs is different. > > > > The question is whether generic call handling works (esp. call_expr_flags > > works correctly - the argument compare should work fine already). > > > > Tom - care to update the patch? > > > > I agree, the current patch is conservative and we can do better. > But I think it's wiser to do that as a stage1 follow-up, and commit this > conservative patch for stage4. Is that acceptable? Then just defer it for stage1 completely. If a problem pops up with GCC 5 we can backport the proper patch together with a testcase. Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-20 11:59 ` Richard Biener @ 2015-02-22 13:13 ` Tom de Vries 2015-02-23 10:19 ` Richard Biener 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-22 13:13 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 1295 bytes --] On 20-02-15 12:54, Richard Biener wrote: > On Thu, 19 Feb 2015, Tom de Vries wrote: > >> On 19-02-15 14:07, Richard Biener wrote: >>> On Thu, 19 Feb 2015, Jakub Jelinek wrote: >>> >>>> On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: >>>>> I'd call it a bug though, and we do have internal fns in >>>>> generic already thus the issue is latent (with ubsan at least). >>>>> >>>>> Which means ok for trunk now. >>>> >>>> But the patch should better handle the internal calls right. >>>> I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, >>>> or if both are NULL and CALL_EXPR_IFN is different, or if >>>> call_expr_nargs is different. >>> >>> The question is whether generic call handling works (esp. call_expr_flags >>> works correctly - the argument compare should work fine already). >>> >>> Tom - care to update the patch? >>> >> >> I agree, the current patch is conservative and we can do betterns, - . >> But I think it's wiser to do that as a stage1 follow-up, and commit this >> conservative patch for stage4. Is that acceptable? > > Then just defer it for stage1 completely. If a problem pops up with > GCC 5 we can backport the proper patch together with a testcase. > Updated patch according to Jakub's comments, retested. OK for stage1? Thanks, - Tom [-- Attachment #2: 0004-Handle-internal_fn-in-operand_equal_p.patch --] [-- Type: text/x-patch, Size: 1836 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> * fold-const.c (operand_equal_p): Handle INTERNAL_FNs. * calls.c (call_expr_flags): Same. --- gcc/calls.c | 2 ++ gcc/fold-const.c | 23 +++++++++++++++++++---- 2 files changed, 21 insertions(+), 4 deletions(-) diff --git a/gcc/calls.c b/gcc/calls.c index ec44624..2919464 100644 --- a/gcc/calls.c +++ b/gcc/calls.c @@ -847,6 +847,8 @@ call_expr_flags (const_tree t) if (decl) flags = flags_from_decl_or_type (decl); + else if (CALL_EXPR_FN (t) == NULL_TREE) + flags = internal_fn_flags (CALL_EXPR_IFN (t)); else { t = TREE_TYPE (CALL_EXPR_FN (t)); diff --git a/gcc/fold-const.c b/gcc/fold-const.c index 8377120..3013adb 100644 --- a/gcc/fold-const.c +++ b/gcc/fold-const.c @@ -3032,11 +3032,26 @@ operand_equal_p (const_tree arg0, const_tree arg1, unsigned int flags) switch (TREE_CODE (arg0)) { case CALL_EXPR: - /* 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), - flags)) + if ((CALL_EXPR_FN (arg0) == NULL_TREE) + != (CALL_EXPR_FN (arg1) == NULL_TREE)) + /* If not both CALL_EXPRs are either internal or normal function + functions, then they are not equal. */ return 0; + else if (CALL_EXPR_FN (arg0) == NULL_TREE) + { + /* If the CALL_EXPRs call different internal functions, then they + are not equal. */ + if (CALL_EXPR_IFN (arg0) != CALL_EXPR_IFN (arg1)) + return 0; + } + else + { + /* If the CALL_EXPRs call different functions, then they are not + equal. */ + if (! operand_equal_p (CALL_EXPR_FN (arg0), CALL_EXPR_FN (arg1), + flags)) + return 0; + } { unsigned int cef = call_expr_flags (arg0); -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-22 13:13 ` Tom de Vries @ 2015-02-23 10:19 ` Richard Biener 2015-02-24 13:09 ` Fwd: " Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Richard Biener @ 2015-02-23 10:19 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Sun, 22 Feb 2015, Tom de Vries wrote: > On 20-02-15 12:54, Richard Biener wrote: > > On Thu, 19 Feb 2015, Tom de Vries wrote: > > > > > On 19-02-15 14:07, Richard Biener wrote: > > > > On Thu, 19 Feb 2015, Jakub Jelinek wrote: > > > > > > > > > On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > > > > > > I'd call it a bug though, and we do have internal fns in > > > > > > generic already thus the issue is latent (with ubsan at least). > > > > > > > > > > > > Which means ok for trunk now. > > > > > > > > > > But the patch should better handle the internal calls right. > > > > > I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, > > > > > or if both are NULL and CALL_EXPR_IFN is different, or if > > > > > call_expr_nargs is different. > > > > > > > > The question is whether generic call handling works (esp. > > > > call_expr_flags > > > > works correctly - the argument compare should work fine already). > > > > > > > > Tom - care to update the patch? > > > > > > > > > > I agree, the current patch is conservative and we can do betterns, > - > . > > > But I think it's wiser to do that as a stage1 follow-up, and commit this > > > conservative patch for stage4. Is that acceptable? > > > > Then just defer it for stage1 completely. If a problem pops up with > > GCC 5 we can backport the proper patch together with a testcase. > > > > Updated patch according to Jakub's comments, retested. > > OK for stage1? Ok. Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Fwd: Re: [PATCH][4/5] Handle internal_fn in operand_equal_p 2015-02-23 10:19 ` Richard Biener @ 2015-02-24 13:09 ` Tom de Vries 0 siblings, 0 replies; 39+ messages in thread From: Tom de Vries @ 2015-02-24 13:09 UTC (permalink / raw) To: GCC Patches [ forwarding. for some reason, this email didn't make it to gcc-patches ml archive ] -------- Forwarded Message -------- Subject: Re: [PATCH][4/5] Handle internal_fn in operand_equal_p Date: Mon, 23 Feb 2015 10:03:34 +0100 From: Richard Biener <rguenther@suse.de> To: Tom de Vries <Tom_deVries@mentor.com> CC: Jakub Jelinek <jakub@redhat.com>, GCC Patches <gcc-patches@gcc.gnu.org>, Michael Matz <matz@suse.de> On Sun, 22 Feb 2015, Tom de Vries wrote: > On 20-02-15 12:54, Richard Biener wrote: > > On Thu, 19 Feb 2015, Tom de Vries wrote: > > > > > On 19-02-15 14:07, Richard Biener wrote: > > > > On Thu, 19 Feb 2015, Jakub Jelinek wrote: > > > > > > > > > On Thu, Feb 19, 2015 at 01:44:46PM +0100, Richard Biener wrote: > > > > > > I'd call it a bug though, and we do have internal fns in > > > > > > generic already thus the issue is latent (with ubsan at least). > > > > > > > > > > > > Which means ok for trunk now. > > > > > > > > > > But the patch should better handle the internal calls right. > > > > > I.e. return 0 only if only one, not both CALL_EXPR_FNs are NULL, > > > > > or if both are NULL and CALL_EXPR_IFN is different, or if > > > > > call_expr_nargs is different. > > > > > > > > The question is whether generic call handling works (esp. > > > > call_expr_flags > > > > works correctly - the argument compare should work fine already). > > > > > > > > Tom - care to update the patch? > > > > > > > > > > I agree, the current patch is conservative and we can do betterns, > - > . > > > But I think it's wiser to do that as a stage1 follow-up, and commit this > > > conservative patch for stage4. Is that acceptable? > > > > Then just defer it for stage1 completely. If a problem pops up with > > GCC 5 we can backport the proper patch together with a testcase. > > > > Updated patch according to Jakub's comments, retested. > > OK for stage1? Ok. Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH][5/5] Postpone expanding va_arg until pass_stdarg 2015-02-19 10:48 [stage1] Postpone expanding va_arg until pass_stdarg Tom de Vries ` (3 preceding siblings ...) 2015-02-19 11:37 ` [PATCH][4/5] Handle internal_fn in operand_equal_p Tom de Vries @ 2015-02-19 12:08 ` Tom de Vries 2015-02-19 13:05 ` Richard Biener 4 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-19 12:08 UTC (permalink / raw) To: Jakub Jelinek, Richard Biener; +Cc: GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 2366 bytes --] On 19-02-15 11:29, Tom de Vries wrote: > Hi, > > I'm posting this patch series for stage1: > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > - 0002-Add-gimple_find_sub_bbs.patch > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > - 0004-Handle-internal_fn-in-operand_equal_p.patch > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > The patch series - based on Michael's initial patch - postpones expanding va_arg > until pass_stdarg, which makes pass_stdarg more robust. > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > unix/-m32 testing. > > I'll post the patches in reply to this email. > This patch postpones expanding va_arg until pass_stdarg. We add a new internal function IFN_VA_ARG. During gimplification, we map VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified in to a gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into actual code. There are a few implementation details worth mentioning: - passing the type beyond gimplification is done by adding a NULL pointer- to-type to IFN_VA_ARG. - there is special handling for IFN_VA_ARG that would be most suited to be placed in gimplify_va_arg_expr. However, that function lacks the scope for the special handling, so it's placed awkwardly in gimplify_modify_expr. - there's special handling in case the va_arg type is variable-sized. gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR IFN_VA_ARG for variable-sized types. However, this is gimplified into a gimple_call which does not have the possibility to wrap it's result in a WITH_SIZE_EXPR. So we're adding the size argument of the WITH_SIZE_EXPR as argument to IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the subsequent gimplify_assign will generate a memcpy if necessary. - when gimplifying the va_arg argument ap, it may not be addressable. So gimplification will generate a copy ap.1 = ap, and use &ap.1 as argument. This means that we have to copy back the ap.1 value to ap after IFN_VA_ARG. The copy is classified by the va_list_gpr/fpr_size optimization as an escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 update is because of that. OK for stage1? Thanks, - Tom [-- Attachment #2: 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch --] [-- Type: text/x-patch, Size: 15499 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> Michael Matz <matz@suse.de> * gimple-iterator.c (update_modified_stmts): Remove static. * gimple-iterator.h (update_modified_stmts): Declare. * gimplify.c (gimplify_modify_expr): Handle IFN_VA_ARG. (gimplify_va_arg_internal): New function. (gimplify_va_arg_expr): Use IFN_VA_ARG. * gimplify.h (gimplify_va_arg_internal): Declare. * internal-fn.c (expand_VA_ARG): New unreachable function. * internal-fn.def (VA_ARG): New DEF_INTERNAL_FN. * tree-stdarg.c (gimple_call_ifn_va_arg_p, expand_ifn_va_arg): New function. (pass_stdarg::execute): Call expand_ifn_va_arg. * gcc.dg/tree-ssa/stdarg-2.c: Change f15 scan-tree-dump for target x86_64-*-*. --- gcc/gimple-iterator.c | 2 +- gcc/gimple-iterator.h | 1 + gcc/gimplify.c | 109 +++++++++++++++++++++-------- gcc/gimplify.h | 2 + gcc/internal-fn.c | 9 +++ gcc/internal-fn.def | 1 + gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c | 4 +- gcc/tree-stdarg.c | 115 +++++++++++++++++++++++++++++-- 8 files changed, 208 insertions(+), 35 deletions(-) diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c index a322390..df29123 100644 --- a/gcc/gimple-iterator.c +++ b/gcc/gimple-iterator.c @@ -72,7 +72,7 @@ update_modified_stmt (gimple stmt) /* Mark the statements in SEQ as modified, and update them. */ -static void +void update_modified_stmts (gimple_seq seq) { gimple_stmt_iterator gsi; diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h index 6be88dd..ab5759e 100644 --- a/gcc/gimple-iterator.h +++ b/gcc/gimple-iterator.h @@ -90,6 +90,7 @@ extern basic_block gsi_insert_seq_on_edge_immediate (edge, gimple_seq); extern void gsi_commit_edge_inserts (void); extern void gsi_commit_one_edge_insert (edge, basic_block *); extern gphi_iterator gsi_start_phis (basic_block); +extern void update_modified_stmts (gimple_seq); /* Return a new iterator pointing to GIMPLE_SEQ's first statement. */ diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 1353ada..932200e 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. */ @@ -9351,36 +9424,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/gimplify.h b/gcc/gimplify.h index 615925c..bad8e0f 100644 --- a/gcc/gimplify.h +++ b/gcc/gimplify.h @@ -82,6 +82,8 @@ extern void gimplify_function_tree (tree); extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *, gimple_seq *); gimple gimplify_assign (tree, tree, gimple_seq *); +extern tree gimplify_va_arg_internal (tree, tree, location_t, gimple_seq *, + gimple_seq *); /* Return true if gimplify_one_sizepos doesn't need to gimplify expr (when in TYPE_SIZE{,_UNIT} and similar type/decl size/bitsize diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index e402825..0053ed9 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -1972,6 +1972,15 @@ expand_BUILTIN_EXPECT (gcall *stmt) emit_move_insn (target, val); } +/* IFN_VA_ARG is supposed to be expanded at pass_stdarg. So this dummy function + should never be called. */ + +static void +expand_VA_ARG (gcall *stmt ATTRIBUTE_UNUSED) +{ + gcc_unreachable (); +} + /* Routines to expand each internal function, indexed by function number. Each routine has the prototype: diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 032ce6c..f557c64 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -62,3 +62,4 @@ DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (TSAN_FUNC_EXIT, ECF_NOVOPS | ECF_LEAF | ECF_NOTHROW, NULL) +DEF_INTERNAL_FN (VA_ARG, 0, NULL) diff --git a/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c b/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c index fe39da3..5a74280 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c @@ -288,9 +288,9 @@ f15 (int i, ...) f15_1 (ap); va_end (ap); } -/* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { { i?86-*-* x86_64-*-* } && { ! { ia32 || llp64 } } } } } } */ +/* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { { i?86-*-* } && { ! { ia32 || llp64 } } } } } } */ /* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { powerpc*-*-linux* && { powerpc_fprs && ilp32 } } } } } */ -/* { dg-final { scan-tree-dump "f15: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" { target alpha*-*-linux* } } } */ +/* { dg-final { scan-tree-dump "f15: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" { target { { alpha*-*-linux* } || { { x86_64-*-* } && { ! { ia32 || llp64 } } } } } } } */ /* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save 1 GPR units and 2 FPR units" "stdarg" { target s390*-*-linux* } } } */ /* { dg-final { scan-tree-dump-not "f15: va_list escapes 0, needs to save 0 GPR units" "stdarg" { target { { i?86-*-* x86_64-*-* } && ia32 } } } } */ /* { dg-final { scan-tree-dump-not "f15: va_list escapes 0, needs to save 0 GPR units" "stdarg" { target ia64-*-* } } } */ diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c index 5eac56a..9dc5fe5 100644 --- a/gcc/tree-stdarg.c +++ b/gcc/tree-stdarg.c @@ -52,11 +52,14 @@ 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-cfg.h" #include "tree-pass.h" #include "tree-stdarg.h" @@ -1016,6 +1019,101 @@ finish: } } +/* Return true if STMT is IFN_VA_ARG. */ + +static bool +gimple_call_ifn_va_arg_p (gimple stmt) +{ + return (is_gimple_call (stmt) + && gimple_call_internal_p (stmt) + && gimple_call_internal_fn (stmt) == IFN_VA_ARG); +} + +/* Expand IFN_VA_ARGs in FUN. */ + +static void +expand_ifn_va_arg (function *fun) +{ + bool modified = false; + basic_block bb; + gimple_stmt_iterator i; + + FOR_EACH_BB_FN (bb, fun) + for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) + { + gimple stmt = gsi_stmt (i); + tree ap, expr, lhs, type; + gimple_seq pre = NULL, post = NULL; + + if (!gimple_call_ifn_va_arg_p (stmt)) + continue; + + modified = true; + + type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 1))); + 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); + + lhs = gimple_call_lhs (stmt); + if (lhs != NULL_TREE) + { + gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type)); + + if (gimple_call_num_args (stmt) == 3) + { + /* We've transported the size of with WITH_SIZE_EXPR here as + the 3rd argument of the internal fn call. Now reinstate + it. */ + tree size = gimple_call_arg (stmt, 2); + expr = build2 (WITH_SIZE_EXPR, TREE_TYPE (expr), expr, size); + } + + /* We use gimplify_assign here, rather than gimple_build_assign, + because gimple_assign knows how to deal with variable-sized + types.*/ + gimplify_assign (lhs, expr, &pre); + } + + pop_gimplify_context (NULL); + + gimple_seq_add_seq (&pre, post); + update_modified_stmts (pre); + + /* Add the sequence after IFN_VA_ARG. This splits the bb right + after IFN_VA_ARG, and adds the sequence in one or more new bbs + inbetween. */ + gimple_find_sub_bbs (pre, &i); + + /* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the + bb. */ + gsi_remove (&i, true); + gcc_assert (gsi_end_p (i)); + + /* We're walking here into the bbs which contain the expansion of + IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs + expanding. We could try to skip walking these bbs, perhaps by + walking backwards over gimples and bbs. */ + break; + } + +#if ENABLE_CHECKING + FOR_EACH_BB_FN (bb, fun) + for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) + gcc_assert (!gimple_call_ifn_va_arg_p (gsi_stmt (i))); +#endif + + if (modified) + { + free_dominance_info (CDI_DOMINATORS); + update_ssa (TODO_update_ssa); + } +} + namespace { const pass_data pass_data_stdarg = @@ -1039,11 +1137,13 @@ public: {} /* opt_pass methods: */ - virtual bool gate (function *fun) + virtual bool gate (function *) { - return (flag_stdarg_opt - /* This optimization is only for stdarg functions. */ - && fun->stdarg != 0); + /* Always run this pass, in order to expand va_arg internal_fns. We + also need to do that if fun->stdarg == 0, because a va_arg may also + occur in a function without varargs, f.i. if when passing a va_list to + another function. */ + return true; } virtual unsigned int execute (function *); @@ -1053,7 +1153,12 @@ public: unsigned int pass_stdarg::execute (function *fun) { - optimize_va_list_gpr_fpr_size (fun); + expand_ifn_va_arg (fun); + + if (flag_stdarg_opt + /* This optimization is only for stdarg functions. */ + && fun->stdarg != 0) + optimize_va_list_gpr_fpr_size (fun); return 0; } -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][5/5] Postpone expanding va_arg until pass_stdarg 2015-02-19 12:08 ` [PATCH][5/5] Postpone expanding va_arg until pass_stdarg Tom de Vries @ 2015-02-19 13:05 ` Richard Biener 2015-02-22 13:24 ` Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Richard Biener @ 2015-02-19 13:05 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 19 Feb 2015, Tom de Vries wrote: > On 19-02-15 11:29, Tom de Vries wrote: > > Hi, > > > > I'm posting this patch series for stage1: > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > - 0002-Add-gimple_find_sub_bbs.patch > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > The patch series - based on Michael's initial patch - postpones expanding > > va_arg > > until pass_stdarg, which makes pass_stdarg more robust. > > > > Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and > > unix/-m32 testing. > > > > I'll post the patches in reply to this email. > > > > This patch postpones expanding va_arg until pass_stdarg. > > We add a new internal function IFN_VA_ARG. During gimplification, we map > VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified in to a > gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into actual > code. > > There are a few implementation details worth mentioning: > - passing the type beyond gimplification is done by adding a NULL pointer- > to-type to IFN_VA_ARG. > - there is special handling for IFN_VA_ARG that would be most suited to be > placed in gimplify_va_arg_expr. However, that function lacks the scope for > the special handling, so it's placed awkwardly in gimplify_modify_expr. > - there's special handling in case the va_arg type is variable-sized. > gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR IFN_VA_ARG for > variable-sized types. However, this is gimplified into a gimple_call which > does not have the possibility to wrap it's result in a WITH_SIZE_EXPR. So > we're adding the size argument of the WITH_SIZE_EXPR as argument to > IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the > gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the subsequent > gimplify_assign will generate a memcpy if necessary. > - when gimplifying the va_arg argument ap, it may not be addressable. So > gimplification will generate a copy ap.1 = ap, and use &ap.1 as argument. > This means that we have to copy back the ap.1 value to ap after IFN_VA_ARG. > The copy is classified by the va_list_gpr/fpr_size optimization as an > escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 update is > because of that. > > OK for stage1? Looks mostly good, though it looks like with -O0 this doesn't delay lowering of va-arg and thus won't "fix" offloading. Can you instead introduce a PROP_gimple_lva, provide it by the stdarg pass and add a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run that runs of !PROP_gimple_lva (and also provides it), and require PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for the complex stuff to get an idea what needs to be touched) Thanks, Richard. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][5/5] Postpone expanding va_arg until pass_stdarg 2015-02-19 13:05 ` Richard Biener @ 2015-02-22 13:24 ` Tom de Vries 2015-02-23 9:03 ` Michael Matz 2015-03-10 15:31 ` [PING] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg Tom de Vries 0 siblings, 2 replies; 39+ messages in thread From: Tom de Vries @ 2015-02-22 13:24 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 3568 bytes --] On 19-02-15 14:03, Richard Biener wrote: > On Thu, 19 Feb 2015, Tom de Vries wrote: > >> On 19-02-15 11:29, Tom de Vries wrote: >>> Hi, >>> >>> I'm posting this patch series for stage1: >>> - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch >>> - 0002-Add-gimple_find_sub_bbs.patch >>> - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch >>> - 0004-Handle-internal_fn-in-operand_equal_p.patch >>> - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch >>> >>> The patch series - based on Michael's initial patch - postpones expanding >>> va_arg >>> until pass_stdarg, which makes pass_stdarg more robust. >>> >>> Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and >>> unix/-m32 testing. >>> >>> I'll post the patches in reply to this email. >>> >> >> This patch postpones expanding va_arg until pass_stdarg. >> >> We add a new internal function IFN_VA_ARG. During gimplification, we map >> VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified in to a >> gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into actual >> code. >> >> There are a few implementation details worth mentioning: >> - passing the type beyond gimplification is done by adding a NULL pointer- >> to-type to IFN_VA_ARG. >> - there is special handling for IFN_VA_ARG that would be most suited to be >> placed in gimplify_va_arg_expr. However, that function lacks the scope for >> the special handling, so it's placed awkwardly in gimplify_modify_expr. >> - there's special handling in case the va_arg type is variable-sized. >> gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR IFN_VA_ARG for >> variable-sized types. However, this is gimplified into a gimple_call which >> does not have the possibility to wrap it's result in a WITH_SIZE_EXPR. So >> we're adding the size argument of the WITH_SIZE_EXPR as argument to >> IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the >> gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the subsequent >> gimplify_assign will generate a memcpy if necessary. >> - when gimplifying the va_arg argument ap, it may not be addressable. So >> gimplification will generate a copy ap.1 = ap, and use &ap.1 as argument. >> This means that we have to copy back the ap.1 value to ap after IFN_VA_ARG. >> The copy is classified by the va_list_gpr/fpr_size optimization as an >> escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 update is >> because of that. >> >> OK for stage1? > > Looks mostly good, though it looks like with -O0 this doesn't delay > lowering of va-arg and thus won't "fix" offloading. Can you instead > introduce a PROP_gimple_lva, provide it by the stdarg pass and add > a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run > that runs of !PROP_gimple_lva (and also provides it), and require > PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for > the complex stuff to get an idea what needs to be touched) > Updated according to comments. Furthermore (having updated the patch series to recent trunk), I'm dropping the ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there relates to this patch. Retested as before. OK for stage1? Btw, I'm wondering if as run-time optimization we can tentatively set PROP_gimple_lva at the start of the gimple pass, and unset it in gimplify_va_arg_expr. That way we would avoid the loop in expand_ifn_va_arg_1 (over all bbs and gimples) in functions without va_arg. Thanks, - Tom [-- Attachment #2: 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch --] [-- Type: text/x-patch, Size: 20341 bytes --] 2015-02-17 Tom de Vries <tom@codesourcery.com> Michael Matz <matz@suse.de> * gimple-iterator.c (update_modified_stmts): Remove static. * gimple-iterator.h (update_modified_stmts): Declare. * gimplify.c (gimplify_modify_expr): Handle IFN_VA_ARG. (gimplify_va_arg_internal): New function. (gimplify_va_arg_expr): Use IFN_VA_ARG. * gimplify.h (gimplify_va_arg_internal): Declare. * internal-fn.c (expand_VA_ARG): New unreachable function. * internal-fn.def (VA_ARG): New DEF_INTERNAL_FN. * tree-stdarg.c (gimple_call_ifn_va_arg_p, expand_ifn_va_arg_1) (expand_ifn_va_arg): New function. (pass_data_stdarg): Add PROP_gimple_lva to properties_provided field. (pass_stdarg::execute): Call expand_ifn_va_arg. (pass_data_lower_vaarg): New pass_data. (pass_lower_vaarg): New gimple_opt_pass. (pass_lower_vaarg::gate, pass_lower_vaarg::execute) (make_pass_lower_vaarg): New function. * cfgexpand.c (pass_data_expand): Add PROP_gimple_lva to properties_required field. * passes.def (all_passes): Add pass_lower_vaarg. * tree-pass.h (PROP_gimple_lva): Add define. (make_pass_lower_vaarg): Declare. * gcc.dg/tree-ssa/stdarg-2.c: Change f15 scan-tree-dump for target x86_64-*-*. --- gcc/cfgexpand.c | 3 +- gcc/gimple-iterator.c | 2 +- gcc/gimple-iterator.h | 1 + gcc/gimplify.c | 111 ++++++++++++++----- gcc/gimplify.h | 2 + gcc/internal-fn.c | 9 ++ gcc/internal-fn.def | 1 + gcc/passes.def | 1 + gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c | 4 +- gcc/tree-pass.h | 2 + gcc/tree-stdarg.c | 184 ++++++++++++++++++++++++++++--- 11 files changed, 273 insertions(+), 47 deletions(-) diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 7dfe1f6..af5a652 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -5860,7 +5860,8 @@ const pass_data pass_data_expand = TV_EXPAND, /* tv_id */ ( PROP_ssa | PROP_gimple_leh | PROP_cfg | PROP_gimple_lcx - | PROP_gimple_lvec ), /* properties_required */ + | PROP_gimple_lvec + | PROP_gimple_lva), /* properties_required */ PROP_rtl, /* properties_provided */ ( PROP_ssa | PROP_trees ), /* properties_destroyed */ 0, /* todo_flags_start */ diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c index a322390..df29123 100644 --- a/gcc/gimple-iterator.c +++ b/gcc/gimple-iterator.c @@ -72,7 +72,7 @@ update_modified_stmt (gimple stmt) /* Mark the statements in SEQ as modified, and update them. */ -static void +void update_modified_stmts (gimple_seq seq) { gimple_stmt_iterator gsi; diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h index 6be88dd..ab5759e 100644 --- a/gcc/gimple-iterator.h +++ b/gcc/gimple-iterator.h @@ -90,6 +90,7 @@ extern basic_block gsi_insert_seq_on_edge_immediate (edge, gimple_seq); extern void gsi_commit_edge_inserts (void); extern void gsi_commit_one_edge_insert (edge, basic_block *); extern gphi_iterator gsi_start_phis (basic_block); +extern void update_modified_stmts (gimple_seq); /* Return a new iterator pointing to GIMPLE_SEQ's first statement. */ diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 1353ada..8ac6a35 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,16 +9327,53 @@ 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. */ enum gimplify_status -gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p) +gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, + gimple_seq *post_p ATTRIBUTE_UNUSED) { tree promoted_type, have_va_type; tree valist = TREE_OPERAND (*expr_p, 0); tree type = TREE_TYPE (*expr_p); - tree t; + tree t, tag, ap; location_t loc = EXPR_LOCATION (*expr_p); /* Verify that valist is of the proper type. */ @@ -9351,36 +9425,13 @@ 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 - { - /* 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); - if (!targetm.gimplify_va_arg_expr) - /* FIXME: Once most targets are converted we should merely - assert this is non-null. */ - return GS_ALL_DONE; + /* 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); - *expr_p = targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p); - return GS_OK; - } + return GS_OK; } /* Build a new GIMPLE_ASSIGN tuple and append it to the end of *SEQ_P. diff --git a/gcc/gimplify.h b/gcc/gimplify.h index 615925c..bad8e0f 100644 --- a/gcc/gimplify.h +++ b/gcc/gimplify.h @@ -82,6 +82,8 @@ extern void gimplify_function_tree (tree); extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *, gimple_seq *); gimple gimplify_assign (tree, tree, gimple_seq *); +extern tree gimplify_va_arg_internal (tree, tree, location_t, gimple_seq *, + gimple_seq *); /* Return true if gimplify_one_sizepos doesn't need to gimplify expr (when in TYPE_SIZE{,_UNIT} and similar type/decl size/bitsize diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c index e402825..0053ed9 100644 --- a/gcc/internal-fn.c +++ b/gcc/internal-fn.c @@ -1972,6 +1972,15 @@ expand_BUILTIN_EXPECT (gcall *stmt) emit_move_insn (target, val); } +/* IFN_VA_ARG is supposed to be expanded at pass_stdarg. So this dummy function + should never be called. */ + +static void +expand_VA_ARG (gcall *stmt ATTRIBUTE_UNUSED) +{ + gcc_unreachable (); +} + /* Routines to expand each internal function, indexed by function number. Each routine has the prototype: diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def index 032ce6c..f557c64 100644 --- a/gcc/internal-fn.def +++ b/gcc/internal-fn.def @@ -62,3 +62,4 @@ DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) DEF_INTERNAL_FN (TSAN_FUNC_EXIT, ECF_NOVOPS | ECF_LEAF | ECF_NOTHROW, NULL) +DEF_INTERNAL_FN (VA_ARG, 0, NULL) diff --git a/gcc/passes.def b/gcc/passes.def index 2bc5dcd..b9d396f 100644 --- a/gcc/passes.def +++ b/gcc/passes.def @@ -342,6 +342,7 @@ along with GCC; see the file COPYING3. If not see NEXT_PASS (pass_tm_edges); POP_INSERT_PASSES () NEXT_PASS (pass_vtable_verify); + NEXT_PASS (pass_lower_vaarg); NEXT_PASS (pass_lower_vector); NEXT_PASS (pass_lower_complex_O0); NEXT_PASS (pass_asan_O0); diff --git a/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c b/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c index fe39da3..5a74280 100644 --- a/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c +++ b/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c @@ -288,9 +288,9 @@ f15 (int i, ...) f15_1 (ap); va_end (ap); } -/* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { { i?86-*-* x86_64-*-* } && { ! { ia32 || llp64 } } } } } } */ +/* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { { i?86-*-* } && { ! { ia32 || llp64 } } } } } } */ /* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { powerpc*-*-linux* && { powerpc_fprs && ilp32 } } } } } */ -/* { dg-final { scan-tree-dump "f15: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" { target alpha*-*-linux* } } } */ +/* { dg-final { scan-tree-dump "f15: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" { target { { alpha*-*-linux* } || { { x86_64-*-* } && { ! { ia32 || llp64 } } } } } } } */ /* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save 1 GPR units and 2 FPR units" "stdarg" { target s390*-*-linux* } } } */ /* { dg-final { scan-tree-dump-not "f15: va_list escapes 0, needs to save 0 GPR units" "stdarg" { target { { i?86-*-* x86_64-*-* } && ia32 } } } } */ /* { dg-final { scan-tree-dump-not "f15: va_list escapes 0, needs to save 0 GPR units" "stdarg" { target ia64-*-* } } } */ diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h index b59ae7a..bcefb3e 100644 --- a/gcc/tree-pass.h +++ b/gcc/tree-pass.h @@ -221,6 +221,7 @@ protected: #define PROP_loops (1 << 11) /* preserve loop structures */ #define PROP_gimple_lvec (1 << 12) /* lowered vector */ #define PROP_gimple_eomp (1 << 13) /* no OpenMP directives */ +#define PROP_gimple_lva (1 << 14) /* No va_arg internal function. */ #define PROP_trees \ (PROP_gimple_any | PROP_gimple_lcf | PROP_gimple_leh | PROP_gimple_lomp) @@ -588,6 +589,7 @@ extern gimple_opt_pass *make_pass_early_inline (gcc::context *ctxt); extern gimple_opt_pass *make_pass_inline_parameters (gcc::context *ctxt); extern gimple_opt_pass *make_pass_update_address_taken (gcc::context *ctxt); extern gimple_opt_pass *make_pass_convert_switch (gcc::context *ctxt); +extern gimple_opt_pass *make_pass_lower_vaarg (gcc::context *ctxt); /* Current optimization pass. */ extern opt_pass *current_pass; diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c index 8d221a4..16a9e2c 100644 --- a/gcc/tree-stdarg.c +++ b/gcc/tree-stdarg.c @@ -52,11 +52,14 @@ 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-cfg.h" #include "tree-pass.h" #include "tree-stdarg.h" @@ -1016,6 +1019,112 @@ finish: } } +/* Return true if STMT is IFN_VA_ARG. */ + +static bool +gimple_call_ifn_va_arg_p (gimple stmt) +{ + return (is_gimple_call (stmt) + && gimple_call_internal_p (stmt) + && gimple_call_internal_fn (stmt) == IFN_VA_ARG); +} + +/* Expand IFN_VA_ARGs in FUN. */ + +static void +expand_ifn_va_arg_1 (function *fun) +{ + bool modified = false; + basic_block bb; + gimple_stmt_iterator i; + + FOR_EACH_BB_FN (bb, fun) + for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) + { + gimple stmt = gsi_stmt (i); + tree ap, expr, lhs, type; + gimple_seq pre = NULL, post = NULL; + + if (!gimple_call_ifn_va_arg_p (stmt)) + continue; + + modified = true; + + type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 1))); + 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); + + lhs = gimple_call_lhs (stmt); + if (lhs != NULL_TREE) + { + gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type)); + + if (gimple_call_num_args (stmt) == 3) + { + /* We've transported the size of with WITH_SIZE_EXPR here as + the 3rd argument of the internal fn call. Now reinstate + it. */ + tree size = gimple_call_arg (stmt, 2); + expr = build2 (WITH_SIZE_EXPR, TREE_TYPE (expr), expr, size); + } + + /* We use gimplify_assign here, rather than gimple_build_assign, + because gimple_assign knows how to deal with variable-sized + types. */ + gimplify_assign (lhs, expr, &pre); + } + + pop_gimplify_context (NULL); + + gimple_seq_add_seq (&pre, post); + update_modified_stmts (pre); + + /* Add the sequence after IFN_VA_ARG. This splits the bb right + after IFN_VA_ARG, and adds the sequence in one or more new bbs + inbetween. */ + gimple_find_sub_bbs (pre, &i); + + /* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the + bb. */ + gsi_remove (&i, true); + gcc_assert (gsi_end_p (i)); + + /* We're walking here into the bbs which contain the expansion of + IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs + expanding. We could try to skip walking these bbs, perhaps by + walking backwards over gimples and bbs. */ + break; + } + + if (!modified) + return; + + free_dominance_info (CDI_DOMINATORS); + update_ssa (TODO_update_ssa); +} + +/* Expand IFN_VA_ARGs in FUN, if necessary. */ + +static void +expand_ifn_va_arg (function *fun) +{ + if ((fun->curr_properties & PROP_gimple_lva) == 0) + expand_ifn_va_arg_1 (fun); + +#if ENABLE_CHECKING + basic_block bb; + gimple_stmt_iterator i; + FOR_EACH_BB_FN (bb, fun) + for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) + gcc_assert (!gimple_call_ifn_va_arg_p (gsi_stmt (i))); +#endif +} + namespace { const pass_data pass_data_stdarg = @@ -1025,7 +1134,7 @@ const pass_data pass_data_stdarg = OPTGROUP_NONE, /* optinfo_flags */ TV_NONE, /* tv_id */ ( PROP_cfg | PROP_ssa ), /* properties_required */ - 0, /* properties_provided */ + PROP_gimple_lva, /* properties_provided */ 0, /* properties_destroyed */ 0, /* todo_flags_start */ 0, /* todo_flags_finish */ @@ -1039,18 +1148,13 @@ public: {} /* opt_pass methods: */ - virtual bool gate (function *fun) + virtual bool gate (function *) { - return (flag_stdarg_opt -#ifdef ACCEL_COMPILER - /* Disable for GCC5 in the offloading compilers, as - va_list and gpr/fpr counter fields are not merged. - In GCC6 when stdarg is lowered late this shouldn't be - an issue. */ - && !in_lto_p -#endif - /* This optimization is only for stdarg functions. */ - && fun->stdarg != 0); + /* Always run this pass, in order to expand va_arg internal_fns. We + also need to do that if fun->stdarg == 0, because a va_arg may also + occur in a function without varargs, f.i. if when passing a va_list to + another function. */ + return true; } virtual unsigned int execute (function *); @@ -1060,7 +1164,14 @@ public: unsigned int pass_stdarg::execute (function *fun) { - optimize_va_list_gpr_fpr_size (fun); + /* TODO: Postpone expand_ifn_va_arg till after + optimize_va_list_gpr_fpr_size. */ + expand_ifn_va_arg (fun); + + if (flag_stdarg_opt + /* This optimization is only for stdarg functions. */ + && fun->stdarg != 0) + optimize_va_list_gpr_fpr_size (fun); return 0; } @@ -1072,3 +1183,50 @@ make_pass_stdarg (gcc::context *ctxt) { return new pass_stdarg (ctxt); } + +namespace { + +const pass_data pass_data_lower_vaarg = +{ + GIMPLE_PASS, /* type */ + "lower_vaarg", /* name */ + OPTGROUP_NONE, /* optinfo_flags */ + TV_NONE, /* tv_id */ + ( PROP_cfg | PROP_ssa ), /* properties_required */ + PROP_gimple_lva, /* properties_provided */ + 0, /* properties_destroyed */ + 0, /* todo_flags_start */ + 0, /* todo_flags_finish */ +}; + +class pass_lower_vaarg : public gimple_opt_pass +{ +public: + pass_lower_vaarg (gcc::context *ctxt) + : gimple_opt_pass (pass_data_lower_vaarg, ctxt) + {} + + /* opt_pass methods: */ + virtual bool gate (function *) + { + return (cfun->curr_properties & PROP_gimple_lva) == 0; + } + + virtual unsigned int execute (function *); + +}; // class pass_lower_vaarg + +unsigned int +pass_lower_vaarg::execute (function *fun) +{ + expand_ifn_va_arg (fun); + return 0; +} + +} // anon namespace + +gimple_opt_pass * +make_pass_lower_vaarg (gcc::context *ctxt) +{ + return new pass_lower_vaarg (ctxt); +} -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][5/5] Postpone expanding va_arg until pass_stdarg 2015-02-22 13:24 ` Tom de Vries @ 2015-02-23 9:03 ` Michael Matz 2015-02-23 10:36 ` Tom de Vries 2015-03-10 15:31 ` [PING] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg Tom de Vries 1 sibling, 1 reply; 39+ messages in thread From: Michael Matz @ 2015-02-23 9:03 UTC (permalink / raw) To: Tom de Vries; +Cc: Richard Biener, Jakub Jelinek, GCC Patches Hi, On Sun, 22 Feb 2015, Tom de Vries wrote: > Btw, I'm wondering if as run-time optimization we can tentatively set > PROP_gimple_lva at the start of the gimple pass, and unset it in > gimplify_va_arg_expr. That way we would avoid the loop in > expand_ifn_va_arg_1 (over all bbs and gimples) in functions without > va_arg. That makes sense. Ciao, Michael. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][5/5] Postpone expanding va_arg until pass_stdarg 2015-02-23 9:03 ` Michael Matz @ 2015-02-23 10:36 ` Tom de Vries 2015-02-24 8:26 ` Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-23 10:36 UTC (permalink / raw) To: Michael Matz; +Cc: Richard Biener, Jakub Jelinek, GCC Patches [-- Attachment #1: Type: text/plain, Size: 464 bytes --] On 23-02-15 09:26, Michael Matz wrote: > Hi, > > On Sun, 22 Feb 2015, Tom de Vries wrote: > >> Btw, I'm wondering if as run-time optimization we can tentatively set >> PROP_gimple_lva at the start of the gimple pass, and unset it in >> gimplify_va_arg_expr. That way we would avoid the loop in >> expand_ifn_va_arg_1 (over all bbs and gimples) in functions without >> va_arg. > > That makes sense. > I'll put this follow-up patch through testing. Thanks, - Tom [-- Attachment #2: 0006-Set-PROP_gimple_lva-for-functions-without-IFN_VA_ARG.patch --] [-- Type: text/x-patch, Size: 2329 bytes --] 2015-02-23 Tom de Vries <tom@codesourcery.com> * gimplify.c (gimplify_function_tree): Tentatively set PROP_gimple_lva in cfun->curr_properties. (gimplify_va_arg_expr): Clear PROP_gimple_lva in cfun->curr_properties if we generate an IFN_VA_ARG. * tree-inline.c (expand_call_inline): Reset PROP_gimple_lva in dest function if PROP_gimple_lva is not set in src function. --- gcc/gimplify.c | 10 +++++++++- gcc/tree-inline.c | 8 ++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 8ac6a35..497e2fa 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -9218,6 +9218,10 @@ gimplify_function_tree (tree fndecl) else push_struct_function (fndecl); + /* Tentatively set PROP_gimple_lva here, and reset it in gimplify_va_arg_expr + if necessary. */ + cfun->curr_properties |= PROP_gimple_lva; + for (parm = DECL_ARGUMENTS (fndecl); parm ; parm = DECL_CHAIN (parm)) { /* Preliminarily mark non-addressed complex variables as eligible @@ -9312,7 +9316,7 @@ gimplify_function_tree (tree fndecl) } DECL_SAVED_TREE (fndecl) = NULL_TREE; - cfun->curr_properties = PROP_gimple_any; + cfun->curr_properties |= PROP_gimple_any; pop_cfun (); } @@ -9431,6 +9435,10 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, tag = build_int_cst (build_pointer_type (type), 0); *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap, tag); + /* Clear the tentatively set PROP_gimple_lva, to indicate that IFN_VA_ARG + needs to be expanded. */ + cfun->curr_properties &= ~PROP_gimple_lva; + return GS_OK; } diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index d8abe03..7dfef4f 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -4524,6 +4524,14 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id) id->src_cfun = DECL_STRUCT_FUNCTION (fn); id->call_stmt = stmt; + /* If the the src function contains an IFN_VA_ARG, then so will the dst + function after inlining. */ + if ((id->src_cfun->curr_properties & PROP_gimple_lva) == 0) + { + struct function *dst_cfun = DECL_STRUCT_FUNCTION (id->dst_fn); + dst_cfun->curr_properties &= ~PROP_gimple_lva; + } + gcc_assert (!id->src_cfun->after_inlining); id->entry_bb = bb; -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH][5/5] Postpone expanding va_arg until pass_stdarg 2015-02-23 10:36 ` Tom de Vries @ 2015-02-24 8:26 ` Tom de Vries 2015-03-10 15:31 ` [PING] [PATCH][5b/5] Avoid running expand_ifn_va_arg_1 in functions without va_arg Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-02-24 8:26 UTC (permalink / raw) To: Richard Biener, Jakub Jelinek; +Cc: Michael Matz, GCC Patches [-- Attachment #1: Type: text/plain, Size: 619 bytes --] On 23-02-15 11:09, Tom de Vries wrote: > On 23-02-15 09:26, Michael Matz wrote: >> Hi, >> >> On Sun, 22 Feb 2015, Tom de Vries wrote: >> >>> Btw, I'm wondering if as run-time optimization we can tentatively set >>> PROP_gimple_lva at the start of the gimple pass, and unset it in >>> gimplify_va_arg_expr. That way we would avoid the loop in >>> expand_ifn_va_arg_1 (over all bbs and gimples) in functions without >>> va_arg. >> >> That makes sense. >> > > I'll put this follow-up patch through testing. Bootstrapped and reg-tested as before together with the rest of the patch series. OK for stage1? Thanks, - Tom [-- Attachment #2: 0006-Set-PROP_gimple_lva-for-functions-without-IFN_VA_ARG.patch --] [-- Type: text/x-patch, Size: 2329 bytes --] 2015-02-23 Tom de Vries <tom@codesourcery.com> * gimplify.c (gimplify_function_tree): Tentatively set PROP_gimple_lva in cfun->curr_properties. (gimplify_va_arg_expr): Clear PROP_gimple_lva in cfun->curr_properties if we generate an IFN_VA_ARG. * tree-inline.c (expand_call_inline): Reset PROP_gimple_lva in dest function if PROP_gimple_lva is not set in src function. --- gcc/gimplify.c | 10 +++++++++- gcc/tree-inline.c | 8 ++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/gcc/gimplify.c b/gcc/gimplify.c index 8ac6a35..497e2fa 100644 --- a/gcc/gimplify.c +++ b/gcc/gimplify.c @@ -9218,6 +9218,10 @@ gimplify_function_tree (tree fndecl) else push_struct_function (fndecl); + /* Tentatively set PROP_gimple_lva here, and reset it in gimplify_va_arg_expr + if necessary. */ + cfun->curr_properties |= PROP_gimple_lva; + for (parm = DECL_ARGUMENTS (fndecl); parm ; parm = DECL_CHAIN (parm)) { /* Preliminarily mark non-addressed complex variables as eligible @@ -9312,7 +9316,7 @@ gimplify_function_tree (tree fndecl) } DECL_SAVED_TREE (fndecl) = NULL_TREE; - cfun->curr_properties = PROP_gimple_any; + cfun->curr_properties |= PROP_gimple_any; pop_cfun (); } @@ -9431,6 +9435,10 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, tag = build_int_cst (build_pointer_type (type), 0); *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap, tag); + /* Clear the tentatively set PROP_gimple_lva, to indicate that IFN_VA_ARG + needs to be expanded. */ + cfun->curr_properties &= ~PROP_gimple_lva; + return GS_OK; } diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c index d8abe03..7dfef4f 100644 --- a/gcc/tree-inline.c +++ b/gcc/tree-inline.c @@ -4524,6 +4524,14 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id) id->src_cfun = DECL_STRUCT_FUNCTION (fn); id->call_stmt = stmt; + /* If the the src function contains an IFN_VA_ARG, then so will the dst + function after inlining. */ + if ((id->src_cfun->curr_properties & PROP_gimple_lva) == 0) + { + struct function *dst_cfun = DECL_STRUCT_FUNCTION (id->dst_fn); + dst_cfun->curr_properties &= ~PROP_gimple_lva; + } + gcc_assert (!id->src_cfun->after_inlining); id->entry_bb = bb; -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PING] [PATCH][5b/5] Avoid running expand_ifn_va_arg_1 in functions without va_arg 2015-02-24 8:26 ` Tom de Vries @ 2015-03-10 15:31 ` Tom de Vries 2015-04-16 8:50 ` [PING^2] " Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-03-10 15:31 UTC (permalink / raw) To: Richard Biener, Jakub Jelinek; +Cc: Michael Matz, GCC Patches [stage1 ping] [was: Postpone expanding va_arg until pass_stdarg] On 24-02-15 07:48, Tom de Vries wrote: > On 23-02-15 11:09, Tom de Vries wrote: >> On 23-02-15 09:26, Michael Matz wrote: >>> Hi, >>> >>> On Sun, 22 Feb 2015, Tom de Vries wrote: >>> >>>> Btw, I'm wondering if as run-time optimization we can tentatively set >>>> PROP_gimple_lva at the start of the gimple pass, and unset it in >>>> gimplify_va_arg_expr. That way we would avoid the loop in >>>> expand_ifn_va_arg_1 (over all bbs and gimples) in functions without >>>> va_arg. >>> >>> That makes sense. >>> >> >> I'll put this follow-up patch through testing. > > Bootstrapped and reg-tested as before together with the rest of the patch series. > > OK for stage1? > Ping. Thanks, - Tom > 0006-Set-PROP_gimple_lva-for-functions-without-IFN_VA_ARG.patch > > > 2015-02-23 Tom de Vries<tom@codesourcery.com> > > * gimplify.c (gimplify_function_tree): Tentatively set PROP_gimple_lva > in cfun->curr_properties. > (gimplify_va_arg_expr): Clear PROP_gimple_lva in cfun->curr_properties > if we generate an IFN_VA_ARG. > * tree-inline.c (expand_call_inline): Reset PROP_gimple_lva in dest > function if PROP_gimple_lva is not set in src function. > --- > gcc/gimplify.c | 10 +++++++++- > gcc/tree-inline.c | 8 ++++++++ > 2 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/gcc/gimplify.c b/gcc/gimplify.c > index 8ac6a35..497e2fa 100644 > --- a/gcc/gimplify.c > +++ b/gcc/gimplify.c > @@ -9218,6 +9218,10 @@ gimplify_function_tree (tree fndecl) > else > push_struct_function (fndecl); > > + /* Tentatively set PROP_gimple_lva here, and reset it in gimplify_va_arg_expr > + if necessary. */ > + cfun->curr_properties |= PROP_gimple_lva; > + > for (parm = DECL_ARGUMENTS (fndecl); parm ; parm = DECL_CHAIN (parm)) > { > /* Preliminarily mark non-addressed complex variables as eligible > @@ -9312,7 +9316,7 @@ gimplify_function_tree (tree fndecl) > } > > DECL_SAVED_TREE (fndecl) = NULL_TREE; > - cfun->curr_properties = PROP_gimple_any; > + cfun->curr_properties |= PROP_gimple_any; > > pop_cfun (); > } > @@ -9431,6 +9435,10 @@ gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, > tag = build_int_cst (build_pointer_type (type), 0); > *expr_p = build_call_expr_internal_loc (loc, IFN_VA_ARG, type, 2, ap, tag); > > + /* Clear the tentatively set PROP_gimple_lva, to indicate that IFN_VA_ARG > + needs to be expanded. */ > + cfun->curr_properties &= ~PROP_gimple_lva; > + > return GS_OK; > } > > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c > index d8abe03..7dfef4f 100644 > --- a/gcc/tree-inline.c > +++ b/gcc/tree-inline.c > @@ -4524,6 +4524,14 @@ expand_call_inline (basic_block bb, gimple stmt, copy_body_data *id) > id->src_cfun = DECL_STRUCT_FUNCTION (fn); > id->call_stmt = stmt; > > + /* If the the src function contains an IFN_VA_ARG, then so will the dst > + function after inlining. */ > + if ((id->src_cfun->curr_properties & PROP_gimple_lva) == 0) > + { > + struct function *dst_cfun = DECL_STRUCT_FUNCTION (id->dst_fn); > + dst_cfun->curr_properties &= ~PROP_gimple_lva; > + } > + > gcc_assert (!id->src_cfun->after_inlining); > > id->entry_bb = bb; > -- 1.9.1 > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PING^2] [PATCH][5b/5] Avoid running expand_ifn_va_arg_1 in functions without va_arg 2015-03-10 15:31 ` [PING] [PATCH][5b/5] Avoid running expand_ifn_va_arg_1 in functions without va_arg Tom de Vries @ 2015-04-16 8:50 ` Tom de Vries 2015-04-16 8:56 ` Richard Biener 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-04-16 8:50 UTC (permalink / raw) To: Richard Biener, Jakub Jelinek; +Cc: Michael Matz, GCC Patches [stage1 ping^2] On 10-03-15 16:31, Tom de Vries wrote: > [stage1 ping] > [was: Postpone expanding va_arg until pass_stdarg] > On 24-02-15 07:48, Tom de Vries wrote: >> On 23-02-15 11:09, Tom de Vries wrote: >>> On 23-02-15 09:26, Michael Matz wrote: >>>> Hi, >>>> >>>> On Sun, 22 Feb 2015, Tom de Vries wrote: >>>> >>>>> Btw, I'm wondering if as run-time optimization we can tentatively set >>>>> PROP_gimple_lva at the start of the gimple pass, and unset it in >>>>> gimplify_va_arg_expr. That way we would avoid the loop in >>>>> expand_ifn_va_arg_1 (over all bbs and gimples) in functions without >>>>> va_arg. >>>> >>>> That makes sense. >>>> >>> >>> I'll put this follow-up patch through testing. >> >> Bootstrapped and reg-tested as before together with the rest of the patch series. >> >> OK for stage1? >> > > Ping. > Ping again. Patch originally posted at: https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01402.html . Thanks, - Tom ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5b/5] Avoid running expand_ifn_va_arg_1 in functions without va_arg 2015-04-16 8:50 ` [PING^2] " Tom de Vries @ 2015-04-16 8:56 ` Richard Biener 0 siblings, 0 replies; 39+ messages in thread From: Richard Biener @ 2015-04-16 8:56 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, Michael Matz, GCC Patches On Thu, 16 Apr 2015, Tom de Vries wrote: > [stage1 ping^2] > On 10-03-15 16:31, Tom de Vries wrote: > > [stage1 ping] > > [was: Postpone expanding va_arg until pass_stdarg] > > On 24-02-15 07:48, Tom de Vries wrote: > > > On 23-02-15 11:09, Tom de Vries wrote: > > > > On 23-02-15 09:26, Michael Matz wrote: > > > > > Hi, > > > > > > > > > > On Sun, 22 Feb 2015, Tom de Vries wrote: > > > > > > > > > > > Btw, I'm wondering if as run-time optimization we can tentatively > > > > > > set > > > > > > PROP_gimple_lva at the start of the gimple pass, and unset it in > > > > > > gimplify_va_arg_expr. That way we would avoid the loop in > > > > > > expand_ifn_va_arg_1 (over all bbs and gimples) in functions without > > > > > > va_arg. > > > > > > > > > > That makes sense. > > > > > > > > > > > > > I'll put this follow-up patch through testing. > > > > > > Bootstrapped and reg-tested as before together with the rest of the patch > > > series. > > > > > > OK for stage1? > > > > > > > Ping. > > > > Ping again. > > Patch originally posted at: > https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01402.html . Ok. Thanks, Richard. > Thanks, > - Tom > > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild, Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PING] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-02-22 13:24 ` Tom de Vries 2015-02-23 9:03 ` Michael Matz @ 2015-03-10 15:31 ` Tom de Vries 2015-04-16 8:50 ` [PING^2] " Tom de Vries 1 sibling, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-03-10 15:31 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC Patches, Michael Matz [stage1 ping] On 22-02-15 14:13, Tom de Vries wrote: > On 19-02-15 14:03, Richard Biener wrote: >> On Thu, 19 Feb 2015, Tom de Vries wrote: >> >>> On 19-02-15 11:29, Tom de Vries wrote: >>>> Hi, >>>> >>>> I'm posting this patch series for stage1: >>>> - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch >>>> - 0002-Add-gimple_find_sub_bbs.patch >>>> - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch >>>> - 0004-Handle-internal_fn-in-operand_equal_p.patch >>>> - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch >>>> >>>> The patch series - based on Michael's initial patch - postpones expanding >>>> va_arg >>>> until pass_stdarg, which makes pass_stdarg more robust. >>>> >>>> Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and >>>> unix/-m32 testing. >>>> >>>> I'll post the patches in reply to this email. >>>> >>> >>> This patch postpones expanding va_arg until pass_stdarg. >>> >>> We add a new internal function IFN_VA_ARG. During gimplification, we map >>> VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified in to a >>> gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into actual >>> code. >>> >>> There are a few implementation details worth mentioning: >>> - passing the type beyond gimplification is done by adding a NULL pointer- >>> to-type to IFN_VA_ARG. >>> - there is special handling for IFN_VA_ARG that would be most suited to be >>> placed in gimplify_va_arg_expr. However, that function lacks the scope for >>> the special handling, so it's placed awkwardly in gimplify_modify_expr. >>> - there's special handling in case the va_arg type is variable-sized. >>> gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR IFN_VA_ARG for >>> variable-sized types. However, this is gimplified into a gimple_call which >>> does not have the possibility to wrap it's result in a WITH_SIZE_EXPR. So >>> we're adding the size argument of the WITH_SIZE_EXPR as argument to >>> IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the >>> gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the subsequent >>> gimplify_assign will generate a memcpy if necessary. >>> - when gimplifying the va_arg argument ap, it may not be addressable. So >>> gimplification will generate a copy ap.1 = ap, and use &ap.1 as argument. >>> This means that we have to copy back the ap.1 value to ap after IFN_VA_ARG. >>> The copy is classified by the va_list_gpr/fpr_size optimization as an >>> escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 update is >>> because of that. >>> >>> OK for stage1? >> >> Looks mostly good, though it looks like with -O0 this doesn't delay >> lowering of va-arg and thus won't "fix" offloading. Can you instead >> introduce a PROP_gimple_lva, provide it by the stdarg pass and add >> a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run >> that runs of !PROP_gimple_lva (and also provides it), and require >> PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for >> the complex stuff to get an idea what needs to be touched) >> > > Updated according to comments. > > Furthermore (having updated the patch series to recent trunk), I'm dropping the > ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there relates to this > patch. > > Retested as before. > > OK for stage1? > Ping. > Btw, I'm wondering if as run-time optimization we can tentatively set > PROP_gimple_lva at the start of the gimple pass, and unset it in > gimplify_va_arg_expr. That way we would avoid the loop in expand_ifn_va_arg_1 > (over all bbs and gimples) in functions without va_arg. > Taken care of in follow-up patch 5b. Thanks, - Tom > 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > 2015-02-17 Tom de Vries<tom@codesourcery.com> > Michael Matz<matz@suse.de> > > * gimple-iterator.c (update_modified_stmts): Remove static. > * gimple-iterator.h (update_modified_stmts): Declare. > * gimplify.c (gimplify_modify_expr): Handle IFN_VA_ARG. > (gimplify_va_arg_internal): New function. > (gimplify_va_arg_expr): Use IFN_VA_ARG. > * gimplify.h (gimplify_va_arg_internal): Declare. > * internal-fn.c (expand_VA_ARG): New unreachable function. > * internal-fn.def (VA_ARG): New DEF_INTERNAL_FN. > * tree-stdarg.c (gimple_call_ifn_va_arg_p, expand_ifn_va_arg_1) > (expand_ifn_va_arg): New function. > (pass_data_stdarg): Add PROP_gimple_lva to properties_provided field. > (pass_stdarg::execute): Call expand_ifn_va_arg. > (pass_data_lower_vaarg): New pass_data. > (pass_lower_vaarg): New gimple_opt_pass. > (pass_lower_vaarg::gate, pass_lower_vaarg::execute) > (make_pass_lower_vaarg): New function. > * cfgexpand.c (pass_data_expand): Add PROP_gimple_lva to > properties_required field. > * passes.def (all_passes): Add pass_lower_vaarg. > * tree-pass.h (PROP_gimple_lva): Add define. > (make_pass_lower_vaarg): Declare. > > * gcc.dg/tree-ssa/stdarg-2.c: Change f15 scan-tree-dump for target > x86_64-*-*. > --- > gcc/cfgexpand.c | 3 +- > gcc/gimple-iterator.c | 2 +- > gcc/gimple-iterator.h | 1 + > gcc/gimplify.c | 111 ++++++++++++++----- > gcc/gimplify.h | 2 + > gcc/internal-fn.c | 9 ++ > gcc/internal-fn.def | 1 + > gcc/passes.def | 1 + > gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c | 4 +- > gcc/tree-pass.h | 2 + > gcc/tree-stdarg.c | 184 ++++++++++++++++++++++++++++--- > 11 files changed, 273 insertions(+), 47 deletions(-) > > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c > index 7dfe1f6..af5a652 100644 > --- a/gcc/cfgexpand.c > +++ b/gcc/cfgexpand.c > @@ -5860,7 +5860,8 @@ const pass_data pass_data_expand = > TV_EXPAND, /* tv_id */ > ( PROP_ssa | PROP_gimple_leh | PROP_cfg > | PROP_gimple_lcx > - | PROP_gimple_lvec ), /* properties_required */ > + | PROP_gimple_lvec > + | PROP_gimple_lva), /* properties_required */ > PROP_rtl, /* properties_provided */ > ( PROP_ssa | PROP_trees ), /* properties_destroyed */ > 0, /* todo_flags_start */ > diff --git a/gcc/gimple-iterator.c b/gcc/gimple-iterator.c > index a322390..df29123 100644 > --- a/gcc/gimple-iterator.c > +++ b/gcc/gimple-iterator.c > @@ -72,7 +72,7 @@ update_modified_stmt (gimple stmt) > > /* Mark the statements in SEQ as modified, and update them. */ > > -static void > +void > update_modified_stmts (gimple_seq seq) > { > gimple_stmt_iterator gsi; > diff --git a/gcc/gimple-iterator.h b/gcc/gimple-iterator.h > index 6be88dd..ab5759e 100644 > --- a/gcc/gimple-iterator.h > +++ b/gcc/gimple-iterator.h > @@ -90,6 +90,7 @@ extern basic_block gsi_insert_seq_on_edge_immediate (edge, gimple_seq); > extern void gsi_commit_edge_inserts (void); > extern void gsi_commit_one_edge_insert (edge, basic_block *); > extern gphi_iterator gsi_start_phis (basic_block); > +extern void update_modified_stmts (gimple_seq); > > /* Return a new iterator pointing to GIMPLE_SEQ's first statement. */ > > diff --git a/gcc/gimplify.c b/gcc/gimplify.c > index 1353ada..8ac6a35 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,16 +9327,53 @@ 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. */ > > enum gimplify_status > -gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p) > +gimplify_va_arg_expr (tree *expr_p, gimple_seq *pre_p, > + gimple_seq *post_p ATTRIBUTE_UNUSED) > { > tree promoted_type, have_va_type; > tree valist = TREE_OPERAND (*expr_p, 0); > tree type = TREE_TYPE (*expr_p); > - tree t; > + tree t, tag, ap; > location_t loc = EXPR_LOCATION (*expr_p); > > /* Verify that valist is of the proper type. */ > @@ -9351,36 +9425,13 @@ 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 > - { > - /* 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); > > - if (!targetm.gimplify_va_arg_expr) > - /* FIXME: Once most targets are converted we should merely > - assert this is non-null. */ > - return GS_ALL_DONE; > + /* 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); > > - *expr_p = targetm.gimplify_va_arg_expr (valist, type, pre_p, post_p); > - return GS_OK; > - } > + return GS_OK; > } > > /* Build a new GIMPLE_ASSIGN tuple and append it to the end of *SEQ_P. > diff --git a/gcc/gimplify.h b/gcc/gimplify.h > index 615925c..bad8e0f 100644 > --- a/gcc/gimplify.h > +++ b/gcc/gimplify.h > @@ -82,6 +82,8 @@ extern void gimplify_function_tree (tree); > extern enum gimplify_status gimplify_va_arg_expr (tree *, gimple_seq *, > gimple_seq *); > gimple gimplify_assign (tree, tree, gimple_seq *); > +extern tree gimplify_va_arg_internal (tree, tree, location_t, gimple_seq *, > + gimple_seq *); > > /* Return true if gimplify_one_sizepos doesn't need to gimplify > expr (when in TYPE_SIZE{,_UNIT} and similar type/decl size/bitsize > diff --git a/gcc/internal-fn.c b/gcc/internal-fn.c > index e402825..0053ed9 100644 > --- a/gcc/internal-fn.c > +++ b/gcc/internal-fn.c > @@ -1972,6 +1972,15 @@ expand_BUILTIN_EXPECT (gcall *stmt) > emit_move_insn (target, val); > } > > +/* IFN_VA_ARG is supposed to be expanded at pass_stdarg. So this dummy function > + should never be called. */ > + > +static void > +expand_VA_ARG (gcall *stmt ATTRIBUTE_UNUSED) > +{ > + gcc_unreachable (); > +} > + > /* Routines to expand each internal function, indexed by function number. > Each routine has the prototype: > > diff --git a/gcc/internal-fn.def b/gcc/internal-fn.def > index 032ce6c..f557c64 100644 > --- a/gcc/internal-fn.def > +++ b/gcc/internal-fn.def > @@ -62,3 +62,4 @@ DEF_INTERNAL_FN (ADD_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (SUB_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (MUL_OVERFLOW, ECF_CONST | ECF_LEAF | ECF_NOTHROW, NULL) > DEF_INTERNAL_FN (TSAN_FUNC_EXIT, ECF_NOVOPS | ECF_LEAF | ECF_NOTHROW, NULL) > +DEF_INTERNAL_FN (VA_ARG, 0, NULL) > diff --git a/gcc/passes.def b/gcc/passes.def > index 2bc5dcd..b9d396f 100644 > --- a/gcc/passes.def > +++ b/gcc/passes.def > @@ -342,6 +342,7 @@ along with GCC; see the file COPYING3. If not see > NEXT_PASS (pass_tm_edges); > POP_INSERT_PASSES () > NEXT_PASS (pass_vtable_verify); > + NEXT_PASS (pass_lower_vaarg); > NEXT_PASS (pass_lower_vector); > NEXT_PASS (pass_lower_complex_O0); > NEXT_PASS (pass_asan_O0); > diff --git a/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c b/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c > index fe39da3..5a74280 100644 > --- a/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c > +++ b/gcc/testsuite/gcc.dg/tree-ssa/stdarg-2.c > @@ -288,9 +288,9 @@ f15 (int i, ...) > f15_1 (ap); > va_end (ap); > } > -/* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { { i?86-*-* x86_64-*-* } && { ! { ia32 || llp64 } } } } } } */ > +/* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { { i?86-*-* } && { ! { ia32 || llp64 } } } } } } */ > /* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save \[148\] GPR units and \[1-9\]\[0-9\]* FPR units" "stdarg" { target { powerpc*-*-linux* && { powerpc_fprs && ilp32 } } } } } */ > -/* { dg-final { scan-tree-dump "f15: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" { target alpha*-*-linux* } } } */ > +/* { dg-final { scan-tree-dump "f15: va_list escapes 1, needs to save all GPR units and all FPR units" "stdarg" { target { { alpha*-*-linux* } || { { x86_64-*-* } && { ! { ia32 || llp64 } } } } } } } */ > /* { dg-final { scan-tree-dump "f15: va_list escapes 0, needs to save 1 GPR units and 2 FPR units" "stdarg" { target s390*-*-linux* } } } */ > /* { dg-final { scan-tree-dump-not "f15: va_list escapes 0, needs to save 0 GPR units" "stdarg" { target { { i?86-*-* x86_64-*-* } && ia32 } } } } */ > /* { dg-final { scan-tree-dump-not "f15: va_list escapes 0, needs to save 0 GPR units" "stdarg" { target ia64-*-* } } } */ > diff --git a/gcc/tree-pass.h b/gcc/tree-pass.h > index b59ae7a..bcefb3e 100644 > --- a/gcc/tree-pass.h > +++ b/gcc/tree-pass.h > @@ -221,6 +221,7 @@ protected: > #define PROP_loops (1 << 11) /* preserve loop structures */ > #define PROP_gimple_lvec (1 << 12) /* lowered vector */ > #define PROP_gimple_eomp (1 << 13) /* no OpenMP directives */ > +#define PROP_gimple_lva (1 << 14) /* No va_arg internal function. */ > > #define PROP_trees \ > (PROP_gimple_any | PROP_gimple_lcf | PROP_gimple_leh | PROP_gimple_lomp) > @@ -588,6 +589,7 @@ extern gimple_opt_pass *make_pass_early_inline (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_inline_parameters (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_update_address_taken (gcc::context *ctxt); > extern gimple_opt_pass *make_pass_convert_switch (gcc::context *ctxt); > +extern gimple_opt_pass *make_pass_lower_vaarg (gcc::context *ctxt); > > /* Current optimization pass. */ > extern opt_pass *current_pass; > diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c > index 8d221a4..16a9e2c 100644 > --- a/gcc/tree-stdarg.c > +++ b/gcc/tree-stdarg.c > @@ -52,11 +52,14 @@ 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-cfg.h" > #include "tree-pass.h" > #include "tree-stdarg.h" > > @@ -1016,6 +1019,112 @@ finish: > } > } > > +/* Return true if STMT is IFN_VA_ARG. */ > + > +static bool > +gimple_call_ifn_va_arg_p (gimple stmt) > +{ > + return (is_gimple_call (stmt) > + && gimple_call_internal_p (stmt) > + && gimple_call_internal_fn (stmt) == IFN_VA_ARG); > +} > + > +/* Expand IFN_VA_ARGs in FUN. */ > + > +static void > +expand_ifn_va_arg_1 (function *fun) > +{ > + bool modified = false; > + basic_block bb; > + gimple_stmt_iterator i; > + > + FOR_EACH_BB_FN (bb, fun) > + for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) > + { > + gimple stmt = gsi_stmt (i); > + tree ap, expr, lhs, type; > + gimple_seq pre = NULL, post = NULL; > + > + if (!gimple_call_ifn_va_arg_p (stmt)) > + continue; > + > + modified = true; > + > + type = TREE_TYPE (TREE_TYPE (gimple_call_arg (stmt, 1))); > + 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); > + > + lhs = gimple_call_lhs (stmt); > + if (lhs != NULL_TREE) > + { > + gcc_assert (useless_type_conversion_p (TREE_TYPE (lhs), type)); > + > + if (gimple_call_num_args (stmt) == 3) > + { > + /* We've transported the size of with WITH_SIZE_EXPR here as > + the 3rd argument of the internal fn call. Now reinstate > + it. */ > + tree size = gimple_call_arg (stmt, 2); > + expr = build2 (WITH_SIZE_EXPR, TREE_TYPE (expr), expr, size); > + } > + > + /* We use gimplify_assign here, rather than gimple_build_assign, > + because gimple_assign knows how to deal with variable-sized > + types. */ > + gimplify_assign (lhs, expr, &pre); > + } > + > + pop_gimplify_context (NULL); > + > + gimple_seq_add_seq (&pre, post); > + update_modified_stmts (pre); > + > + /* Add the sequence after IFN_VA_ARG. This splits the bb right > + after IFN_VA_ARG, and adds the sequence in one or more new bbs > + inbetween. */ > + gimple_find_sub_bbs (pre, &i); > + > + /* Remove the IFN_VA_ARG gimple_call. It's the last stmt in the > + bb. */ > + gsi_remove (&i, true); > + gcc_assert (gsi_end_p (i)); > + > + /* We're walking here into the bbs which contain the expansion of > + IFN_VA_ARG, and will not contain another IFN_VA_ARG that needs > + expanding. We could try to skip walking these bbs, perhaps by > + walking backwards over gimples and bbs. */ > + break; > + } > + > + if (!modified) > + return; > + > + free_dominance_info (CDI_DOMINATORS); > + update_ssa (TODO_update_ssa); > +} > + > +/* Expand IFN_VA_ARGs in FUN, if necessary. */ > + > +static void > +expand_ifn_va_arg (function *fun) > +{ > + if ((fun->curr_properties & PROP_gimple_lva) == 0) > + expand_ifn_va_arg_1 (fun); > + > +#if ENABLE_CHECKING > + basic_block bb; > + gimple_stmt_iterator i; > + FOR_EACH_BB_FN (bb, fun) > + for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) > + gcc_assert (!gimple_call_ifn_va_arg_p (gsi_stmt (i))); > +#endif > +} > + > namespace { > > const pass_data pass_data_stdarg = > @@ -1025,7 +1134,7 @@ const pass_data pass_data_stdarg = > OPTGROUP_NONE, /* optinfo_flags */ > TV_NONE, /* tv_id */ > ( PROP_cfg | PROP_ssa ), /* properties_required */ > - 0, /* properties_provided */ > + PROP_gimple_lva, /* properties_provided */ > 0, /* properties_destroyed */ > 0, /* todo_flags_start */ > 0, /* todo_flags_finish */ > @@ -1039,18 +1148,13 @@ public: > {} > > /* opt_pass methods: */ > - virtual bool gate (function *fun) > + virtual bool gate (function *) > { > - return (flag_stdarg_opt > -#ifdef ACCEL_COMPILER > - /* Disable for GCC5 in the offloading compilers, as > - va_list and gpr/fpr counter fields are not merged. > - In GCC6 when stdarg is lowered late this shouldn't be > - an issue. */ > - && !in_lto_p > -#endif > - /* This optimization is only for stdarg functions. */ > - && fun->stdarg != 0); > + /* Always run this pass, in order to expand va_arg internal_fns. We > + also need to do that if fun->stdarg == 0, because a va_arg may also > + occur in a function without varargs, f.i. if when passing a va_list to > + another function. */ > + return true; > } > > virtual unsigned int execute (function *); > @@ -1060,7 +1164,14 @@ public: > unsigned int > pass_stdarg::execute (function *fun) > { > - optimize_va_list_gpr_fpr_size (fun); > + /* TODO: Postpone expand_ifn_va_arg till after > + optimize_va_list_gpr_fpr_size. */ > + expand_ifn_va_arg (fun); > + > + if (flag_stdarg_opt > + /* This optimization is only for stdarg functions. */ > + && fun->stdarg != 0) > + optimize_va_list_gpr_fpr_size (fun); > > return 0; > } > @@ -1072,3 +1183,50 @@ make_pass_stdarg (gcc::context *ctxt) > { > return new pass_stdarg (ctxt); > } > + > +namespace { > + > +const pass_data pass_data_lower_vaarg = > +{ > + GIMPLE_PASS, /* type */ > + "lower_vaarg", /* name */ > + OPTGROUP_NONE, /* optinfo_flags */ > + TV_NONE, /* tv_id */ > + ( PROP_cfg | PROP_ssa ), /* properties_required */ > + PROP_gimple_lva, /* properties_provided */ > + 0, /* properties_destroyed */ > + 0, /* todo_flags_start */ > + 0, /* todo_flags_finish */ > +}; > + > +class pass_lower_vaarg : public gimple_opt_pass > +{ > +public: > + pass_lower_vaarg (gcc::context *ctxt) > + : gimple_opt_pass (pass_data_lower_vaarg, ctxt) > + {} > + > + /* opt_pass methods: */ > + virtual bool gate (function *) > + { > + return (cfun->curr_properties & PROP_gimple_lva) == 0; > + } > + > + virtual unsigned int execute (function *); > + > +}; // class pass_lower_vaarg > + > +unsigned int > +pass_lower_vaarg::execute (function *fun) > +{ > + expand_ifn_va_arg (fun); > + return 0; > +} > + > +} // anon namespace > + > +gimple_opt_pass * > +make_pass_lower_vaarg (gcc::context *ctxt) > +{ > + return new pass_lower_vaarg (ctxt); > +} > -- 1.9.1 > ^ permalink raw reply [flat|nested] 39+ messages in thread
* [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-03-10 15:31 ` [PING] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg Tom de Vries @ 2015-04-16 8:50 ` Tom de Vries 2015-04-16 8:55 ` Richard Biener 0 siblings, 1 reply; 39+ messages in thread From: Tom de Vries @ 2015-04-16 8:50 UTC (permalink / raw) To: Richard Biener; +Cc: Jakub Jelinek, GCC Patches, Michael Matz [stage1 ping^2] On 10-03-15 16:30, Tom de Vries wrote: > [stage1 ping] > On 22-02-15 14:13, Tom de Vries wrote: >> On 19-02-15 14:03, Richard Biener wrote: >>> On Thu, 19 Feb 2015, Tom de Vries wrote: >>> >>>> On 19-02-15 11:29, Tom de Vries wrote: >>>>> Hi, >>>>> >>>>> I'm posting this patch series for stage1: >>>>> - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch >>>>> - 0002-Add-gimple_find_sub_bbs.patch >>>>> - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch >>>>> - 0004-Handle-internal_fn-in-operand_equal_p.patch >>>>> - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch >>>>> >>>>> The patch series - based on Michael's initial patch - postpones expanding >>>>> va_arg >>>>> until pass_stdarg, which makes pass_stdarg more robust. >>>>> >>>>> Bootstrapped and reg-tested on x86_64 using all languages, with unix/ and >>>>> unix/-m32 testing. >>>>> >>>>> I'll post the patches in reply to this email. >>>>> >>>> >>>> This patch postpones expanding va_arg until pass_stdarg. >>>> >>>> We add a new internal function IFN_VA_ARG. During gimplification, we map >>>> VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified in to a >>>> gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into actual >>>> code. >>>> >>>> There are a few implementation details worth mentioning: >>>> - passing the type beyond gimplification is done by adding a NULL pointer- >>>> to-type to IFN_VA_ARG. >>>> - there is special handling for IFN_VA_ARG that would be most suited to be >>>> placed in gimplify_va_arg_expr. However, that function lacks the scope for >>>> the special handling, so it's placed awkwardly in gimplify_modify_expr. >>>> - there's special handling in case the va_arg type is variable-sized. >>>> gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR IFN_VA_ARG for >>>> variable-sized types. However, this is gimplified into a gimple_call which >>>> does not have the possibility to wrap it's result in a WITH_SIZE_EXPR. So >>>> we're adding the size argument of the WITH_SIZE_EXPR as argument to >>>> IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the >>>> gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the subsequent >>>> gimplify_assign will generate a memcpy if necessary. >>>> - when gimplifying the va_arg argument ap, it may not be addressable. So >>>> gimplification will generate a copy ap.1 = ap, and use &ap.1 as argument. >>>> This means that we have to copy back the ap.1 value to ap after IFN_VA_ARG. >>>> The copy is classified by the va_list_gpr/fpr_size optimization as an >>>> escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 update is >>>> because of that. >>>> >>>> OK for stage1? >>> >>> Looks mostly good, though it looks like with -O0 this doesn't delay >>> lowering of va-arg and thus won't "fix" offloading. Can you instead >>> introduce a PROP_gimple_lva, provide it by the stdarg pass and add >>> a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run >>> that runs of !PROP_gimple_lva (and also provides it), and require >>> PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for >>> the complex stuff to get an idea what needs to be touched) >>> >> >> Updated according to comments. >> >> Furthermore (having updated the patch series to recent trunk), I'm dropping the >> ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there relates to this >> patch. >> >> Retested as before. >> >> OK for stage1? >> > > Ping. Ping again. Patch originally posted at: https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01332.html . Thanks, - Tom >> Btw, I'm wondering if as run-time optimization we can tentatively set >> PROP_gimple_lva at the start of the gimple pass, and unset it in >> gimplify_va_arg_expr. That way we would avoid the loop in expand_ifn_va_arg_1 >> (over all bbs and gimples) in functions without va_arg. >> > > Taken care of in follow-up patch 5b. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-04-16 8:50 ` [PING^2] " Tom de Vries @ 2015-04-16 8:55 ` Richard Biener 2015-04-21 5:32 ` Bin.Cheng 2015-06-09 11:04 ` Alan Lawrence 0 siblings, 2 replies; 39+ messages in thread From: Richard Biener @ 2015-04-16 8:55 UTC (permalink / raw) To: Tom de Vries; +Cc: Jakub Jelinek, GCC Patches, Michael Matz On Thu, 16 Apr 2015, Tom de Vries wrote: > [stage1 ping^2] > On 10-03-15 16:30, Tom de Vries wrote: > > [stage1 ping] > > On 22-02-15 14:13, Tom de Vries wrote: > > > On 19-02-15 14:03, Richard Biener wrote: > > > > On Thu, 19 Feb 2015, Tom de Vries wrote: > > > > > > > > > On 19-02-15 11:29, Tom de Vries wrote: > > > > > > Hi, > > > > > > > > > > > > I'm posting this patch series for stage1: > > > > > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > > > > > - 0002-Add-gimple_find_sub_bbs.patch > > > > > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > > > > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > > > > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > > > > > > > > > The patch series - based on Michael's initial patch - postpones > > > > > > expanding > > > > > > va_arg > > > > > > until pass_stdarg, which makes pass_stdarg more robust. > > > > > > > > > > > > Bootstrapped and reg-tested on x86_64 using all languages, with > > > > > > unix/ and > > > > > > unix/-m32 testing. > > > > > > > > > > > > I'll post the patches in reply to this email. > > > > > > > > > > > > > > > > This patch postpones expanding va_arg until pass_stdarg. > > > > > > > > > > We add a new internal function IFN_VA_ARG. During gimplification, we > > > > > map > > > > > VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified > > > > > in to a > > > > > gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into > > > > > actual > > > > > code. > > > > > > > > > > There are a few implementation details worth mentioning: > > > > > - passing the type beyond gimplification is done by adding a NULL > > > > > pointer- > > > > > to-type to IFN_VA_ARG. > > > > > - there is special handling for IFN_VA_ARG that would be most suited > > > > > to be > > > > > placed in gimplify_va_arg_expr. However, that function lacks the > > > > > scope for > > > > > the special handling, so it's placed awkwardly in > > > > > gimplify_modify_expr. > > > > > - there's special handling in case the va_arg type is variable-sized. > > > > > gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR > > > > > IFN_VA_ARG for > > > > > variable-sized types. However, this is gimplified into a > > > > > gimple_call which > > > > > does not have the possibility to wrap it's result in a > > > > > WITH_SIZE_EXPR. So > > > > > we're adding the size argument of the WITH_SIZE_EXPR as argument to > > > > > IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the > > > > > gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the > > > > > subsequent > > > > > gimplify_assign will generate a memcpy if necessary. > > > > > - when gimplifying the va_arg argument ap, it may not be addressable. > > > > > So > > > > > gimplification will generate a copy ap.1 = ap, and use &ap.1 as > > > > > argument. > > > > > This means that we have to copy back the ap.1 value to ap after > > > > > IFN_VA_ARG. > > > > > The copy is classified by the va_list_gpr/fpr_size optimization as > > > > > an > > > > > escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 > > > > > update is > > > > > because of that. > > > > > > > > > > OK for stage1? > > > > > > > > Looks mostly good, though it looks like with -O0 this doesn't delay > > > > lowering of va-arg and thus won't "fix" offloading. Can you instead > > > > introduce a PROP_gimple_lva, provide it by the stdarg pass and add > > > > a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run > > > > that runs of !PROP_gimple_lva (and also provides it), and require > > > > PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for > > > > the complex stuff to get an idea what needs to be touched) > > > > > > > > > > Updated according to comments. > > > > > > Furthermore (having updated the patch series to recent trunk), I'm > > > dropping the > > > ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there relates > > > to this > > > patch. > > > > > > Retested as before. > > > > > > OK for stage1? > > > > > > > Ping. > > Ping again. > > Patch originally posted at: > https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01332.html . Ok. Thanks, Richard. > Thanks, > - Tom > > > > Btw, I'm wondering if as run-time optimization we can tentatively set > > > PROP_gimple_lva at the start of the gimple pass, and unset it in > > > gimplify_va_arg_expr. That way we would avoid the loop in > > > expand_ifn_va_arg_1 > > > (over all bbs and gimples) in functions without va_arg. > > > > > > > Taken care of in follow-up patch 5b. ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-04-16 8:55 ` Richard Biener @ 2015-04-21 5:32 ` Bin.Cheng 2015-06-09 11:04 ` Alan Lawrence 1 sibling, 0 replies; 39+ messages in thread From: Bin.Cheng @ 2015-04-21 5:32 UTC (permalink / raw) To: Richard Biener; +Cc: Tom de Vries, Jakub Jelinek, GCC Patches, Michael Matz On Thu, Apr 16, 2015 at 4:55 PM, Richard Biener <rguenther@suse.de> wrote: > On Thu, 16 Apr 2015, Tom de Vries wrote: > >> [stage1 ping^2] >> On 10-03-15 16:30, Tom de Vries wrote: >> > [stage1 ping] >> > On 22-02-15 14:13, Tom de Vries wrote: >> > > On 19-02-15 14:03, Richard Biener wrote: >> > > > On Thu, 19 Feb 2015, Tom de Vries wrote: >> > > > >> > > > > On 19-02-15 11:29, Tom de Vries wrote: >> > > > > > Hi, >> > > > > > >> > > > > > I'm posting this patch series for stage1: >> > > > > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch >> > > > > > - 0002-Add-gimple_find_sub_bbs.patch >> > > > > > - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch >> > > > > > - 0004-Handle-internal_fn-in-operand_equal_p.patch >> > > > > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch >> > > > > > >> > > > > > The patch series - based on Michael's initial patch - postpones >> > > > > > expanding >> > > > > > va_arg >> > > > > > until pass_stdarg, which makes pass_stdarg more robust. >> > > > > > >> > > > > > Bootstrapped and reg-tested on x86_64 using all languages, with >> > > > > > unix/ and >> > > > > > unix/-m32 testing. >> > > > > > >> > > > > > I'll post the patches in reply to this email. >> > > > > > >> > > > > >> > > > > This patch postpones expanding va_arg until pass_stdarg. >> > > > > >> > > > > We add a new internal function IFN_VA_ARG. During gimplification, we >> > > > > map >> > > > > VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified >> > > > > in to a >> > > > > gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into >> > > > > actual >> > > > > code. >> > > > > >> > > > > There are a few implementation details worth mentioning: >> > > > > - passing the type beyond gimplification is done by adding a NULL >> > > > > pointer- >> > > > > to-type to IFN_VA_ARG. >> > > > > - there is special handling for IFN_VA_ARG that would be most suited >> > > > > to be >> > > > > placed in gimplify_va_arg_expr. However, that function lacks the >> > > > > scope for >> > > > > the special handling, so it's placed awkwardly in >> > > > > gimplify_modify_expr. >> > > > > - there's special handling in case the va_arg type is variable-sized. >> > > > > gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR >> > > > > IFN_VA_ARG for >> > > > > variable-sized types. However, this is gimplified into a >> > > > > gimple_call which >> > > > > does not have the possibility to wrap it's result in a >> > > > > WITH_SIZE_EXPR. So >> > > > > we're adding the size argument of the WITH_SIZE_EXPR as argument to >> > > > > IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the >> > > > > gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the >> > > > > subsequent >> > > > > gimplify_assign will generate a memcpy if necessary. >> > > > > - when gimplifying the va_arg argument ap, it may not be addressable. >> > > > > So >> > > > > gimplification will generate a copy ap.1 = ap, and use &ap.1 as >> > > > > argument. >> > > > > This means that we have to copy back the ap.1 value to ap after >> > > > > IFN_VA_ARG. >> > > > > The copy is classified by the va_list_gpr/fpr_size optimization as >> > > > > an >> > > > > escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 >> > > > > update is >> > > > > because of that. >> > > > > >> > > > > OK for stage1? >> > > > >> > > > Looks mostly good, though it looks like with -O0 this doesn't delay >> > > > lowering of va-arg and thus won't "fix" offloading. Can you instead >> > > > introduce a PROP_gimple_lva, provide it by the stdarg pass and add >> > > > a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run >> > > > that runs of !PROP_gimple_lva (and also provides it), and require >> > > > PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for >> > > > the complex stuff to get an idea what needs to be touched) >> > > > >> > > >> > > Updated according to comments. >> > > >> > > Furthermore (having updated the patch series to recent trunk), I'm >> > > dropping the >> > > ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there relates >> > > to this >> > > patch. >> > > >> > > Retested as before. >> > > >> > > OK for stage1? >> > > >> > >> > Ping. >> >> Ping again. >> >> Patch originally posted at: >> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01332.html . > > Ok. > > Thanks, > Richard. > >> Thanks, >> - Tom >> >> > > Btw, I'm wondering if as run-time optimization we can tentatively set >> > > PROP_gimple_lva at the start of the gimple pass, and unset it in >> > > gimplify_va_arg_expr. That way we would avoid the loop in >> > > expand_ifn_va_arg_1 >> > > (over all bbs and gimples) in functions without va_arg. >> > > >> > >> > Taken care of in follow-up patch 5b. Hi, This patch causes stdarg test failure on aarch64 for both linux and elf variants. Seems wrong code is generated. FAIL: gcc.c-torture/execute/stdarg-1.c -O0 execution test FAIL: gcc.c-torture/execute/stdarg-1.c -O1 execution test FAIL: gcc.c-torture/execute/stdarg-1.c -O2 execution test FAIL: gcc.c-torture/execute/stdarg-1.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.c-torture/execute/stdarg-1.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test FAIL: gcc.c-torture/execute/stdarg-1.c -O3 -fomit-frame-pointer execution test FAIL: gcc.c-torture/execute/stdarg-1.c -O3 -g execution test FAIL: gcc.c-torture/execute/stdarg-1.c -Os execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O0 execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O1 execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O2 execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O2 -flto -fno-use-linker-plugin -flto-partition=none execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O2 -flto -fuse-linker-plugin -fno-fat-lto-objects execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O3 -fomit-frame-pointer execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O3 -fomit-frame-pointer -funroll-all-loops -finline-functions execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O3 -fomit-frame-pointer -funroll-loops execution test FAIL: gcc.c-torture/execute/va-arg-11.c -O3 -g execution test FAIL: gcc.c-torture/execute/va-arg-11.c -Os execution test It may also causes ICE on arm-none-linux-gnueabihf. compiler exited with status 1 output is: /projects/.../src/gcc/gcc/testsuite/gcc.c-torture/execute/stdarg-2.c: In function 'f3': /projects/.../src/gcc/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]; /projects/.../src/gcc/gcc/testsuite/gcc.c-torture/execute/stdarg-2.c:61:1: internal compiler error: verify_gimple failed 0xae4893 verify_gimple_in_cfg(function*, bool) /projects/.../src/gcc/gcc/tree-cfg.c:5136 0x9e3f2f execute_function_todo /projects/.../src/gcc/gcc/passes.c:1946 0x9e48d3 execute_todo /projects/.../src/gcc/gcc/passes.c:2003 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <http://gcc.gnu.org/bugs.html> for instructions. FAIL: gcc.c-torture/execute/stdarg-2.c -O0 (internal compiler error) FAIL: gcc.c-torture/execute/stdarg-2.c -O0 (test for excess errors) Thanks, bin ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-04-16 8:55 ` Richard Biener 2015-04-21 5:32 ` Bin.Cheng @ 2015-06-09 11:04 ` Alan Lawrence 2015-06-09 11:07 ` Richard Biener 1 sibling, 1 reply; 39+ messages in thread From: Alan Lawrence @ 2015-06-09 11:04 UTC (permalink / raw) To: Richard Biener; +Cc: Tom de Vries, Jakub Jelinek, GCC Patches, Michael Matz Hmmm. One side effect of this is that the line number information available in the target hook gimplify_va_arg_expr, is now just the name of the containing function, rather than the specific use of va_arg. Is there some way to get this more precise location (e.g. gimple_location(stmt) in expand_ifn_va_arg_1, the only caller of said hook)? I don't really want to have to add an extra parameter to the target hook... --Alan Richard Biener wrote: > On Thu, 16 Apr 2015, Tom de Vries wrote: > >> [stage1 ping^2] >> On 10-03-15 16:30, Tom de Vries wrote: >>> [stage1 ping] >>> On 22-02-15 14:13, Tom de Vries wrote: >>>> On 19-02-15 14:03, Richard Biener wrote: >>>>> On Thu, 19 Feb 2015, Tom de Vries wrote: >>>>> >>>>>> On 19-02-15 11:29, Tom de Vries wrote: >>>>>>> Hi, >>>>>>> >>>>>>> I'm posting this patch series for stage1: >>>>>>> - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch >>>>>>> - 0002-Add-gimple_find_sub_bbs.patch >>>>>>> - 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch >>>>>>> - 0004-Handle-internal_fn-in-operand_equal_p.patch >>>>>>> - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch >>>>>>> >>>>>>> The patch series - based on Michael's initial patch - postpones >>>>>>> expanding >>>>>>> va_arg >>>>>>> until pass_stdarg, which makes pass_stdarg more robust. >>>>>>> >>>>>>> Bootstrapped and reg-tested on x86_64 using all languages, with >>>>>>> unix/ and >>>>>>> unix/-m32 testing. >>>>>>> >>>>>>> I'll post the patches in reply to this email. >>>>>>> >>>>>> This patch postpones expanding va_arg until pass_stdarg. >>>>>> >>>>>> We add a new internal function IFN_VA_ARG. During gimplification, we >>>>>> map >>>>>> VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then gimplified >>>>>> in to a >>>>>> gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call into >>>>>> actual >>>>>> code. >>>>>> >>>>>> There are a few implementation details worth mentioning: >>>>>> - passing the type beyond gimplification is done by adding a NULL >>>>>> pointer- >>>>>> to-type to IFN_VA_ARG. >>>>>> - there is special handling for IFN_VA_ARG that would be most suited >>>>>> to be >>>>>> placed in gimplify_va_arg_expr. However, that function lacks the >>>>>> scope for >>>>>> the special handling, so it's placed awkwardly in >>>>>> gimplify_modify_expr. >>>>>> - there's special handling in case the va_arg type is variable-sized. >>>>>> gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR >>>>>> IFN_VA_ARG for >>>>>> variable-sized types. However, this is gimplified into a >>>>>> gimple_call which >>>>>> does not have the possibility to wrap it's result in a >>>>>> WITH_SIZE_EXPR. So >>>>>> we're adding the size argument of the WITH_SIZE_EXPR as argument to >>>>>> IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of the >>>>>> gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the >>>>>> subsequent >>>>>> gimplify_assign will generate a memcpy if necessary. >>>>>> - when gimplifying the va_arg argument ap, it may not be addressable. >>>>>> So >>>>>> gimplification will generate a copy ap.1 = ap, and use &ap.1 as >>>>>> argument. >>>>>> This means that we have to copy back the ap.1 value to ap after >>>>>> IFN_VA_ARG. >>>>>> The copy is classified by the va_list_gpr/fpr_size optimization as >>>>>> an >>>>>> escape, so it inhibits optimization. The tree-ssa/stdarg-2.c f15 >>>>>> update is >>>>>> because of that. >>>>>> >>>>>> OK for stage1? >>>>> Looks mostly good, though it looks like with -O0 this doesn't delay >>>>> lowering of va-arg and thus won't "fix" offloading. Can you instead >>>>> introduce a PROP_gimple_lva, provide it by the stdarg pass and add >>>>> a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run >>>>> that runs of !PROP_gimple_lva (and also provides it), and require >>>>> PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for >>>>> the complex stuff to get an idea what needs to be touched) >>>>> >>>> Updated according to comments. >>>> >>>> Furthermore (having updated the patch series to recent trunk), I'm >>>> dropping the >>>> ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there relates >>>> to this >>>> patch. >>>> >>>> Retested as before. >>>> >>>> OK for stage1? >>>> >>> Ping. >> Ping again. >> >> Patch originally posted at: >> https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01332.html . > > Ok. > > Thanks, > Richard. > >> Thanks, >> - Tom >> >>>> Btw, I'm wondering if as run-time optimization we can tentatively set >>>> PROP_gimple_lva at the start of the gimple pass, and unset it in >>>> gimplify_va_arg_expr. That way we would avoid the loop in >>>> expand_ifn_va_arg_1 >>>> (over all bbs and gimples) in functions without va_arg. >>>> >>> Taken care of in follow-up patch 5b. > > ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-06-09 11:04 ` Alan Lawrence @ 2015-06-09 11:07 ` Richard Biener 2015-06-09 13:03 ` Tom de Vries 0 siblings, 1 reply; 39+ messages in thread From: Richard Biener @ 2015-06-09 11:07 UTC (permalink / raw) To: Alan Lawrence; +Cc: Tom de Vries, Jakub Jelinek, GCC Patches, Michael Matz On Tue, 9 Jun 2015, Alan Lawrence wrote: > Hmmm. One side effect of this is that the line number information available in > the target hook gimplify_va_arg_expr, is now just the name of the containing > function, rather than the specific use of va_arg. Is there some way to get > this more precise location (e.g. gimple_location(stmt) in expand_ifn_va_arg_1, > the only caller of said hook)? I don't really want to have to add an extra > parameter to the target hook... The x86 variant doesn't use any locations but if then the caller of the target hook (expand_ifn_va_arg_1) should assign the IFNs location to all statements expanded from it (it could set input_location to that during the target hook call...) Richard. > --Alan > > Richard Biener wrote: > > On Thu, 16 Apr 2015, Tom de Vries wrote: > > > > > [stage1 ping^2] > > > On 10-03-15 16:30, Tom de Vries wrote: > > > > [stage1 ping] > > > > On 22-02-15 14:13, Tom de Vries wrote: > > > > > On 19-02-15 14:03, Richard Biener wrote: > > > > > > On Thu, 19 Feb 2015, Tom de Vries wrote: > > > > > > > > > > > > > On 19-02-15 11:29, Tom de Vries wrote: > > > > > > > > Hi, > > > > > > > > > > > > > > > > I'm posting this patch series for stage1: > > > > > > > > - 0001-Disable-lang_hooks.gimplify_expr-in-free_lang_data.patch > > > > > > > > - 0002-Add-gimple_find_sub_bbs.patch > > > > > > > > - > > > > > > > > 0003-Factor-optimize_va_list_gpr_fpr_size-out-of-pass_std.patch > > > > > > > > - 0004-Handle-internal_fn-in-operand_equal_p.patch > > > > > > > > - 0005-Postpone-expanding-va_arg-until-pass_stdarg.patch > > > > > > > > > > > > > > > > The patch series - based on Michael's initial patch - postpones > > > > > > > > expanding > > > > > > > > va_arg > > > > > > > > until pass_stdarg, which makes pass_stdarg more robust. > > > > > > > > > > > > > > > > Bootstrapped and reg-tested on x86_64 using all languages, with > > > > > > > > unix/ and > > > > > > > > unix/-m32 testing. > > > > > > > > > > > > > > > > I'll post the patches in reply to this email. > > > > > > > > > > > > > > > This patch postpones expanding va_arg until pass_stdarg. > > > > > > > > > > > > > > We add a new internal function IFN_VA_ARG. During gimplification, > > > > > > > we > > > > > > > map > > > > > > > VA_ARG_EXPR onto a CALL_EXPR with IFN_VA_ARG, which is then > > > > > > > gimplified > > > > > > > in to a > > > > > > > gimple_call. At pass_stdarg, we expand the IFN_VA_ARG gimple_call > > > > > > > into > > > > > > > actual > > > > > > > code. > > > > > > > > > > > > > > There are a few implementation details worth mentioning: > > > > > > > - passing the type beyond gimplification is done by adding a NULL > > > > > > > pointer- > > > > > > > to-type to IFN_VA_ARG. > > > > > > > - there is special handling for IFN_VA_ARG that would be most > > > > > > > suited > > > > > > > to be > > > > > > > placed in gimplify_va_arg_expr. However, that function lacks > > > > > > > the > > > > > > > scope for > > > > > > > the special handling, so it's placed awkwardly in > > > > > > > gimplify_modify_expr. > > > > > > > - there's special handling in case the va_arg type is > > > > > > > variable-sized. > > > > > > > gimplify_modify_expr adds a WITH_SIZE_EXPR to the CALL_EXPR > > > > > > > IFN_VA_ARG for > > > > > > > variable-sized types. However, this is gimplified into a > > > > > > > gimple_call which > > > > > > > does not have the possibility to wrap it's result in a > > > > > > > WITH_SIZE_EXPR. So > > > > > > > we're adding the size argument of the WITH_SIZE_EXPR as > > > > > > > argument to > > > > > > > IFN_VA_ARG, and at expansion in pass_stdarg, wrap the result of > > > > > > > the > > > > > > > gimplification of IFN_VA_ARG in a WITH_SIZE_EXPR, such that the > > > > > > > subsequent > > > > > > > gimplify_assign will generate a memcpy if necessary. > > > > > > > - when gimplifying the va_arg argument ap, it may not be > > > > > > > addressable. > > > > > > > So > > > > > > > gimplification will generate a copy ap.1 = ap, and use &ap.1 as > > > > > > > argument. > > > > > > > This means that we have to copy back the ap.1 value to ap after > > > > > > > IFN_VA_ARG. > > > > > > > The copy is classified by the va_list_gpr/fpr_size optimization > > > > > > > as > > > > > > > an > > > > > > > escape, so it inhibits optimization. The tree-ssa/stdarg-2.c > > > > > > > f15 > > > > > > > update is > > > > > > > because of that. > > > > > > > > > > > > > > OK for stage1? > > > > > > Looks mostly good, though it looks like with -O0 this doesn't delay > > > > > > lowering of va-arg and thus won't "fix" offloading. Can you instead > > > > > > introduce a PROP_gimple_lva, provide it by the stdarg pass and add > > > > > > a pass_lower_vaarg somewhere where pass_lower_complex_O0 is run > > > > > > that runs of !PROP_gimple_lva (and also provides it), and require > > > > > > PROP_gimple_lva by pass_expand? (just look for PROP_gimple_lcx for > > > > > > the complex stuff to get an idea what needs to be touched) > > > > > > > > > > > Updated according to comments. > > > > > > > > > > Furthermore (having updated the patch series to recent trunk), I'm > > > > > dropping the > > > > > ACCEL_COMPILER bit in pass_stdarg::gate. AFAIU the comment there > > > > > relates > > > > > to this > > > > > patch. > > > > > > > > > > Retested as before. > > > > > > > > > > OK for stage1? > > > > > > > > > Ping. > > > Ping again. > > > > > > Patch originally posted at: > > > https://gcc.gnu.org/ml/gcc-patches/2015-02/msg01332.html . > > > > Ok. > > > > Thanks, > > Richard. > > > > > Thanks, > > > - Tom > > > > > > > > Btw, I'm wondering if as run-time optimization we can tentatively set > > > > > PROP_gimple_lva at the start of the gimple pass, and unset it in > > > > > gimplify_va_arg_expr. That way we would avoid the loop in > > > > > expand_ifn_va_arg_1 > > > > > (over all bbs and gimples) in functions without va_arg. > > > > > > > > > Taken care of in follow-up patch 5b. > > > > > > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-06-09 11:07 ` Richard Biener @ 2015-06-09 13:03 ` Tom de Vries 2015-06-09 13:07 ` Richard Biener 2015-06-09 13:30 ` Alan Lawrence 0 siblings, 2 replies; 39+ messages in thread From: Tom de Vries @ 2015-06-09 13:03 UTC (permalink / raw) To: Richard Biener, Alan Lawrence; +Cc: Jakub Jelinek, GCC Patches, Michael Matz [-- Attachment #1: Type: text/plain, Size: 1044 bytes --] On 09/06/15 13:03, Richard Biener wrote: > On Tue, 9 Jun 2015, Alan Lawrence wrote: > >> Hmmm. One side effect of this is that the line number information available in >> the target hook gimplify_va_arg_expr, is now just the name of the containing >> function, rather than the specific use of va_arg. Is there some way to get >> this more precise location (e.g. gimple_location(stmt) in expand_ifn_va_arg_1, >> the only caller of said hook)? I don't really want to have to add an extra >> parameter to the target hook... > > The x86 variant doesn't use any locations but if then the caller of > the target hook (expand_ifn_va_arg_1) should assign the IFNs location > to all statements expanded from it (it could set input_location to > that during the target hook call...) > That seems to work. The scan-assembler-not test in the testcase in attached patch: - fails without the expand_ifn_va_arg_1 patch hunk, and - passes with that hunk. I'll put it through bootstrap and reg-test on x86_64. OK for trunk if that goes well? Thanks, - Tom [-- Attachment #2: 0001-Handle-location-in-expand_ifn_va_arg_1.patch --] [-- Type: text/x-patch, Size: 2113 bytes --] Handle location in expand_ifn_va_arg_1 2015-06-09 Tom de Vries <tom@codesourcery.com> * tree-stdarg.c (expand_ifn_va_arg_1): Handle location. * gcc.target/i386/vararg-loc.c: New test. --- gcc/testsuite/gcc.target/i386/vararg-loc.c | 27 +++++++++++++++++++++++++++ gcc/tree-stdarg.c | 4 ++++ 2 files changed, 31 insertions(+) create mode 100644 gcc/testsuite/gcc.target/i386/vararg-loc.c diff --git a/gcc/testsuite/gcc.target/i386/vararg-loc.c b/gcc/testsuite/gcc.target/i386/vararg-loc.c new file mode 100644 index 0000000..f236fe3 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/vararg-loc.c @@ -0,0 +1,27 @@ +/* { dg-do compile } */ +/* { dg-options "-g -O0" } */ + +#include <stdarg.h> + +int /* 6. */ + /* 7. */ +f (int a, ...) /* 8. */ + /* 9. */ +{ + + int sum = a; + + va_list ap; + + va_start (ap, a); + + sum += va_arg (ap, int); /* 18. */ + + sum += va_arg (ap, int); /* 20. */ + + return sum; +} + +/* { dg-final { scan-assembler-not "\\.loc 1 \[6789\] 0" } } */ +/* { dg-final { scan-assembler-times "\\.loc 1 18 0" 1 } } */ +/* { dg-final { scan-assembler-times "\\.loc 1 20 0" 1 } } */ diff --git a/gcc/tree-stdarg.c b/gcc/tree-stdarg.c index 08d10b5..65fe9f9 100644 --- a/gcc/tree-stdarg.c +++ b/gcc/tree-stdarg.c @@ -1031,6 +1031,7 @@ expand_ifn_va_arg_1 (function *fun) bool modified = false; basic_block bb; gimple_stmt_iterator i; + location_t saved_location; FOR_EACH_BB_FN (bb, fun) for (i = gsi_start_bb (bb); !gsi_end_p (i); gsi_next (&i)) @@ -1051,6 +1052,8 @@ expand_ifn_va_arg_1 (function *fun) ap = build_fold_indirect_ref (ap); push_gimplify_context (false); + saved_location = input_location; + input_location = gimple_location (stmt); /* Make it easier for the backends by protecting the valist argument from multiple evaluations. */ @@ -1081,6 +1084,7 @@ expand_ifn_va_arg_1 (function *fun) else gimplify_expr (&expr, &pre, &post, is_gimple_lvalue, fb_lvalue); + input_location = saved_location; pop_gimplify_context (NULL); gimple_seq_add_seq (&pre, post); -- 1.9.1 ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-06-09 13:03 ` Tom de Vries @ 2015-06-09 13:07 ` Richard Biener 2015-06-09 13:30 ` Alan Lawrence 1 sibling, 0 replies; 39+ messages in thread From: Richard Biener @ 2015-06-09 13:07 UTC (permalink / raw) To: Tom de Vries; +Cc: Alan Lawrence, Jakub Jelinek, GCC Patches, Michael Matz On Tue, 9 Jun 2015, Tom de Vries wrote: > On 09/06/15 13:03, Richard Biener wrote: > > On Tue, 9 Jun 2015, Alan Lawrence wrote: > > > > > Hmmm. One side effect of this is that the line number information > > > available in > > > the target hook gimplify_va_arg_expr, is now just the name of the > > > containing > > > function, rather than the specific use of va_arg. Is there some way to get > > > this more precise location (e.g. gimple_location(stmt) in > > > expand_ifn_va_arg_1, > > > the only caller of said hook)? I don't really want to have to add an extra > > > parameter to the target hook... > > > > The x86 variant doesn't use any locations but if then the caller of > > the target hook (expand_ifn_va_arg_1) should assign the IFNs location > > to all statements expanded from it (it could set input_location to > > that during the target hook call...) > > > > That seems to work. > > The scan-assembler-not test in the testcase in attached patch: > - fails without the expand_ifn_va_arg_1 patch hunk, and > - passes with that hunk. > > I'll put it through bootstrap and reg-test on x86_64. > > OK for trunk if that goes well? Ok. Thanks, Richard. > Thanks, > - Tom > -- Richard Biener <rguenther@suse.de> SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Dilip Upmanyu, Graham Norton, HRB 21284 (AG Nuernberg) ^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PING^2] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg 2015-06-09 13:03 ` Tom de Vries 2015-06-09 13:07 ` Richard Biener @ 2015-06-09 13:30 ` Alan Lawrence 1 sibling, 0 replies; 39+ messages in thread From: Alan Lawrence @ 2015-06-09 13:30 UTC (permalink / raw) To: Tom de Vries; +Cc: Richard Biener, Jakub Jelinek, GCC Patches, Michael Matz Tom de Vries wrote: > On 09/06/15 13:03, Richard Biener wrote: >> On Tue, 9 Jun 2015, Alan Lawrence wrote: >> >>> Hmmm. One side effect of this is that the line number information available in >>> the target hook gimplify_va_arg_expr, is now just the name of the containing >>> function, rather than the specific use of va_arg. Is there some way to get >>> this more precise location (e.g. gimple_location(stmt) in expand_ifn_va_arg_1, >>> the only caller of said hook)? I don't really want to have to add an extra >>> parameter to the target hook... >> The x86 variant doesn't use any locations but if then the caller of >> the target hook (expand_ifn_va_arg_1) should assign the IFNs location >> to all statements expanded from it (it could set input_location to >> that during the target hook call...) >> > > That seems to work. > > The scan-assembler-not test in the testcase in attached patch: > - fails without the expand_ifn_va_arg_1 patch hunk, and > - passes with that hunk. > > I'll put it through bootstrap and reg-test on x86_64. > > OK for trunk if that goes well? > > Thanks, > - Tom That fixes the issue for me (I'm working with on a patch which uses the line info in error output) - thank you very much! Cheers, Alan ^ permalink raw reply [flat|nested] 39+ messages in thread
end of thread, other threads:[~2015-06-09 13:29 UTC | newest] Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-02-19 10:48 [stage1] Postpone expanding va_arg until pass_stdarg Tom de Vries 2015-02-19 10:51 ` [PATCH][1/5] Disable lang_hooks.gimplify_expr in free_lang_data Tom de Vries 2015-02-19 12:41 ` Richard Biener 2015-02-19 10:59 ` [PATCH][2/5] Add gimple_find_sub_bbs Tom de Vries 2015-02-19 12:41 ` Richard Biener 2015-02-19 12:51 ` Jakub Jelinek 2015-02-19 13:03 ` Marek Polacek 2015-02-19 13:31 ` Richard Biener 2015-02-19 13:07 ` Richard Biener 2015-02-22 12:47 ` Tom de Vries 2015-02-19 11:06 ` [PATCH][3/5] Factor optimize_va_list_gpr_fpr_size out of pass_stdarg::execute Tom de Vries 2015-02-19 12:44 ` Richard Biener 2015-02-19 11:37 ` [PATCH][4/5] Handle internal_fn in operand_equal_p Tom de Vries 2015-02-19 12:49 ` Richard Biener 2015-02-19 12:59 ` Jakub Jelinek 2015-02-19 13:09 ` Richard Biener 2015-02-19 15:31 ` Tom de Vries 2015-02-20 11:59 ` Richard Biener 2015-02-22 13:13 ` Tom de Vries 2015-02-23 10:19 ` Richard Biener 2015-02-24 13:09 ` Fwd: " Tom de Vries 2015-02-19 12:08 ` [PATCH][5/5] Postpone expanding va_arg until pass_stdarg Tom de Vries 2015-02-19 13:05 ` Richard Biener 2015-02-22 13:24 ` Tom de Vries 2015-02-23 9:03 ` Michael Matz 2015-02-23 10:36 ` Tom de Vries 2015-02-24 8:26 ` Tom de Vries 2015-03-10 15:31 ` [PING] [PATCH][5b/5] Avoid running expand_ifn_va_arg_1 in functions without va_arg Tom de Vries 2015-04-16 8:50 ` [PING^2] " Tom de Vries 2015-04-16 8:56 ` Richard Biener 2015-03-10 15:31 ` [PING] [PATCH][5a/5] Postpone expanding va_arg until pass_stdarg Tom de Vries 2015-04-16 8:50 ` [PING^2] " Tom de Vries 2015-04-16 8:55 ` Richard Biener 2015-04-21 5:32 ` Bin.Cheng 2015-06-09 11:04 ` Alan Lawrence 2015-06-09 11:07 ` Richard Biener 2015-06-09 13:03 ` Tom de Vries 2015-06-09 13:07 ` Richard Biener 2015-06-09 13:30 ` Alan Lawrence
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).