Re: [PATCH] bitint, v2: Fix up adjustment of large/huge _BitInt arguments of returns_twice calls [PR113466]
On Thu, 14 Mar 2024, Jakub Jelinek wrote: > On Thu, Mar 14, 2024 at 09:59:12AM +0100, Richard Biener wrote: > > On Thu, 14 Mar 2024, Jakub Jelinek wrote: > > > > > On Thu, Mar 14, 2024 at 09:48:45AM +0100, Richard Biener wrote: > > > > Ugh. OK, but I wonder whether we might want to simply delay > > > > fixing the CFG for inserts before returns-twice? Would that make > > > > things less ugly? > > > > > > You mean in lower_call just remember if we added anything before > > > ECF_RETURNS_TWICE call (or say add such stmt into a vector) and > > > then fix it up before the end of the pass? > > > > Yeah, or just walk BBs with abnormal preds, whatever tracking is > > easier. > > Walking all the bbs with abnormal preds would mean I'd have to walk their > whole bodies, because the ECF_RETURNS_TWICE call is no longer at the start. > > The following patch seems to work, ok for trunk if it passes full > bootstrap/regtest? OK. I'll let you decide which variant is better maintainable (I think this one). Thanks, Richard. > 2024-03-14 Jakub Jelinek > > PR tree-optimization/113466 > * gimple-lower-bitint.cc (bitint_large_huge): Add m_returns_twice_calls > member. > (bitint_large_huge::bitint_large_huge): Initialize it. > (bitint_large_huge::~bitint_large_huge): Release it. > (bitint_large_huge::lower_call): Remember ECF_RETURNS_TWICE call stmts > before which at least one statement has been inserted. > (gimple_lower_bitint): Move argument loads before ECF_RETURNS_TWICE > calls to a different block and add corresponding PHIs. > > * gcc.dg/bitint-100.c: New test. > > --- gcc/gimple-lower-bitint.cc.jj 2024-03-13 15:34:29.969725763 +0100 > +++ gcc/gimple-lower-bitint.cc2024-03-14 11:25:07.763169074 +0100 > @@ -419,7 +419,8 @@ struct bitint_large_huge >bitint_large_huge () > : m_names (NULL), m_loads (NULL), m_preserved (NULL), >m_single_use_names (NULL), m_map (NULL), m_vars (NULL), > - m_limb_type (NULL_TREE), m_data (vNULL) {} > + m_limb_type (NULL_TREE), m_data (vNULL), > + m_returns_twice_calls (vNULL) {} > >~bitint_large_huge (); > > @@ -553,6 +554,7 @@ struct bitint_large_huge >unsigned m_bitfld_load; >vec m_data; >unsigned int m_data_cnt; > + vec m_returns_twice_calls; > }; > > bitint_large_huge::~bitint_large_huge () > @@ -565,6 +567,7 @@ bitint_large_huge::~bitint_large_huge () > delete_var_map (m_map); >XDELETEVEC (m_vars); >m_data.release (); > + m_returns_twice_calls.release (); > } > > /* Insert gimple statement G before current location > @@ -5248,6 +5251,7 @@ bitint_large_huge::lower_call (tree obj, >default: > break; >} > + bool returns_twice = (gimple_call_flags (stmt) & ECF_RETURNS_TWICE) != 0; >for (unsigned int i = 0; i < nargs; ++i) > { >tree arg = gimple_call_arg (stmt, i); > @@ -5271,6 +5275,11 @@ bitint_large_huge::lower_call (tree obj, > arg = make_ssa_name (TREE_TYPE (arg)); > gimple *g = gimple_build_assign (arg, v); > gsi_insert_before (, g, GSI_SAME_STMT); > + if (returns_twice) > + { > + m_returns_twice_calls.safe_push (stmt); > + returns_twice = false; > + } > } >gimple_call_set_arg (stmt, i, arg); >if (m_preserved == NULL) > @@ -7091,6 +7100,66 @@ gimple_lower_bitint (void) >if (edge_insertions) > gsi_commit_edge_inserts (); > > + /* Fix up arguments of ECF_RETURNS_TWICE calls. Those were temporarily > + inserted before the call, but that is invalid IL, so move them to the > + right place and add corresponding PHIs. */ > + if (!large_huge.m_returns_twice_calls.is_empty ()) > +{ > + auto_vec arg_stmts; > + while (!large_huge.m_returns_twice_calls.is_empty ()) > + { > + gimple *stmt = large_huge.m_returns_twice_calls.pop (); > + gimple_stmt_iterator gsi = gsi_after_labels (gimple_bb (stmt)); > + while (gsi_stmt (gsi) != stmt) > + { > + arg_stmts.safe_push (gsi_stmt (gsi)); > + gsi_remove (, false); > + } > + gimple *g; > + basic_block bb = NULL; > + edge e = NULL, ead = NULL; > + FOR_EACH_VEC_ELT (arg_stmts, i, g) > + { > + gsi_safe_insert_before (, g); > + if (i == 0) > + { > + bb = gimple_bb (stmt); > + gcc_checking_assert (EDGE_COUNT (bb->preds) == 2); > + e = EDGE_PRED (bb, 0); > + ead = EDGE_PRED (bb, 1); > + if ((ead->flags & EDGE_ABNORMAL) == 0) > + std::swap (e, ead); > + gcc_checking_assert ((e->flags & EDGE_ABNORMAL) == 0 > +&& (ead->flags & EDGE_ABNORMAL)); > + } > + tree lhs = gimple_assign_lhs (g); > + tree arg = lhs; > + gphi *phi = create_phi_node (copy_ssa_name
[PATCH] bitint, v2: Fix up adjustment of large/huge _BitInt arguments of returns_twice calls [PR113466]
On Thu, Mar 14, 2024 at 09:59:12AM +0100, Richard Biener wrote: > On Thu, 14 Mar 2024, Jakub Jelinek wrote: > > > On Thu, Mar 14, 2024 at 09:48:45AM +0100, Richard Biener wrote: > > > Ugh. OK, but I wonder whether we might want to simply delay > > > fixing the CFG for inserts before returns-twice? Would that make > > > things less ugly? > > > > You mean in lower_call just remember if we added anything before > > ECF_RETURNS_TWICE call (or say add such stmt into a vector) and > > then fix it up before the end of the pass? > > Yeah, or just walk BBs with abnormal preds, whatever tracking is > easier. Walking all the bbs with abnormal preds would mean I'd have to walk their whole bodies, because the ECF_RETURNS_TWICE call is no longer at the start. The following patch seems to work, ok for trunk if it passes full bootstrap/regtest? 2024-03-14 Jakub Jelinek PR tree-optimization/113466 * gimple-lower-bitint.cc (bitint_large_huge): Add m_returns_twice_calls member. (bitint_large_huge::bitint_large_huge): Initialize it. (bitint_large_huge::~bitint_large_huge): Release it. (bitint_large_huge::lower_call): Remember ECF_RETURNS_TWICE call stmts before which at least one statement has been inserted. (gimple_lower_bitint): Move argument loads before ECF_RETURNS_TWICE calls to a different block and add corresponding PHIs. * gcc.dg/bitint-100.c: New test. --- gcc/gimple-lower-bitint.cc.jj 2024-03-13 15:34:29.969725763 +0100 +++ gcc/gimple-lower-bitint.cc 2024-03-14 11:25:07.763169074 +0100 @@ -419,7 +419,8 @@ struct bitint_large_huge bitint_large_huge () : m_names (NULL), m_loads (NULL), m_preserved (NULL), m_single_use_names (NULL), m_map (NULL), m_vars (NULL), - m_limb_type (NULL_TREE), m_data (vNULL) {} + m_limb_type (NULL_TREE), m_data (vNULL), + m_returns_twice_calls (vNULL) {} ~bitint_large_huge (); @@ -553,6 +554,7 @@ struct bitint_large_huge unsigned m_bitfld_load; vec m_data; unsigned int m_data_cnt; + vec m_returns_twice_calls; }; bitint_large_huge::~bitint_large_huge () @@ -565,6 +567,7 @@ bitint_large_huge::~bitint_large_huge () delete_var_map (m_map); XDELETEVEC (m_vars); m_data.release (); + m_returns_twice_calls.release (); } /* Insert gimple statement G before current location @@ -5248,6 +5251,7 @@ bitint_large_huge::lower_call (tree obj, default: break; } + bool returns_twice = (gimple_call_flags (stmt) & ECF_RETURNS_TWICE) != 0; for (unsigned int i = 0; i < nargs; ++i) { tree arg = gimple_call_arg (stmt, i); @@ -5271,6 +5275,11 @@ bitint_large_huge::lower_call (tree obj, arg = make_ssa_name (TREE_TYPE (arg)); gimple *g = gimple_build_assign (arg, v); gsi_insert_before (, g, GSI_SAME_STMT); + if (returns_twice) + { + m_returns_twice_calls.safe_push (stmt); + returns_twice = false; + } } gimple_call_set_arg (stmt, i, arg); if (m_preserved == NULL) @@ -7091,6 +7100,66 @@ gimple_lower_bitint (void) if (edge_insertions) gsi_commit_edge_inserts (); + /* Fix up arguments of ECF_RETURNS_TWICE calls. Those were temporarily + inserted before the call, but that is invalid IL, so move them to the + right place and add corresponding PHIs. */ + if (!large_huge.m_returns_twice_calls.is_empty ()) +{ + auto_vec arg_stmts; + while (!large_huge.m_returns_twice_calls.is_empty ()) + { + gimple *stmt = large_huge.m_returns_twice_calls.pop (); + gimple_stmt_iterator gsi = gsi_after_labels (gimple_bb (stmt)); + while (gsi_stmt (gsi) != stmt) + { + arg_stmts.safe_push (gsi_stmt (gsi)); + gsi_remove (, false); + } + gimple *g; + basic_block bb = NULL; + edge e = NULL, ead = NULL; + FOR_EACH_VEC_ELT (arg_stmts, i, g) + { + gsi_safe_insert_before (, g); + if (i == 0) + { + bb = gimple_bb (stmt); + gcc_checking_assert (EDGE_COUNT (bb->preds) == 2); + e = EDGE_PRED (bb, 0); + ead = EDGE_PRED (bb, 1); + if ((ead->flags & EDGE_ABNORMAL) == 0) + std::swap (e, ead); + gcc_checking_assert ((e->flags & EDGE_ABNORMAL) == 0 + && (ead->flags & EDGE_ABNORMAL)); + } + tree lhs = gimple_assign_lhs (g); + tree arg = lhs; + gphi *phi = create_phi_node (copy_ssa_name (arg), bb); + add_phi_arg (phi, arg, e, UNKNOWN_LOCATION); + tree var = create_tmp_reg (TREE_TYPE (arg)); + suppress_warning (var, OPT_Wuninitialized); + arg = get_or_create_ssa_default_def (cfun, var); + SSA_NAME_OCCURS_IN_ABNORMAL_PHI