| Message ID | ZfK1GNhuXFhxWr9q@tucnak |
|---|---|
| State | New |
| Headers | show |
| Series | bitint: Fix up adjustment of large/huge _BitInt arguments of returns_twice calls [PR113466] | expand |
On Thu, 14 Mar 2024, Jakub Jelinek wrote: > Hi! > > This patch (on top of the just posted gsi_safe_insert* fixes patch) > fixes the instrumentation of large/huge _BitInt SSA_NAME arguments of > returns_twice calls. > > In this case it isn't just a matter of using gsi_safe_insert_before instead > of gsi_insert_before, we need to do more. > > One thing is that unlike the asan/ubsan instrumentation which does just some > checking, here we want the statement before the call to load into a SSA_NAME > which is passed to the call. With another edge we need to add a PHI, > with one PHI argument the loaded SSA_NAME, another argument an uninitialized > warning free SSA_NAME and a result and arrange for all 3 SSA_NAMEs to be > preserved (i.e. stay as is, be no longer lowered afterwards). > > Another problem is because edge_before_returns_twice_call may use > copy_ssa_name, we can end up with large/huge _BitInt SSA_NAMEs we don't > really track in the pass. We need an underlying variable for those, but > because of the way they are constructed we can find that easily, we can > use the same underlying variable for the PHI arg from non-EDGE_ABNORMAL > edge as we use for the corresponding PHI result. The ugliest part of > this is growing the partition if it needs to be growed (otherwise it is > just a partition_union). > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 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? Thanks, Richard. > 2024-03-14 Jakub Jelinek <jakub@redhat.com> > > PR tree-optimization/113466 > * gimple-lower-bitint.cc (bitint_large_huge::lower_call): Handle > ECF_RETURNS_TWICE call arguments correctly. > (gimple_lower_bitint): Ignore PHIs where the PHI result is > in m_preserved bitmap. > > * gcc.dg/bitint-100.c: New test. > > --- gcc/gimple-lower-bitint.cc.jj 2024-03-13 13:03:08.120117081 +0100 > +++ gcc/gimple-lower-bitint.cc 2024-03-13 15:05:54.830303524 +0100 > @@ -5248,6 +5248,7 @@ bitint_large_huge::lower_call (tree obj, > default: > break; > } > + int 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); > @@ -5255,6 +5256,8 @@ bitint_large_huge::lower_call (tree obj, > || TREE_CODE (TREE_TYPE (arg)) != BITINT_TYPE > || bitint_precision_kind (TREE_TYPE (arg)) <= bitint_prec_middle) > continue; > + if (m_preserved == NULL) > + m_preserved = BITMAP_ALLOC (NULL); > if (SSA_NAME_IS_DEFAULT_DEF (arg) > && (!SSA_NAME_VAR (arg) || VAR_P (SSA_NAME_VAR (arg)))) > { > @@ -5270,11 +5273,93 @@ bitint_large_huge::lower_call (tree obj, > v = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (arg), v); > arg = make_ssa_name (TREE_TYPE (arg)); > gimple *g = gimple_build_assign (arg, v); > - gsi_insert_before (&gsi, g, GSI_SAME_STMT); > + gsi_safe_insert_before (&gsi, g); > + if (returns_twice) > + { > + basic_block bb = gimple_bb (stmt); > + gcc_checking_assert (EDGE_COUNT (bb->preds) == 2); > + edge 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)); > + if (returns_twice == 1) > + { > + /* edge_before_returns_twice_call can use copy_ssa_name > + for some PHIs, but in that case we need to put it > + into the same partition as the copied SSA_NAME. */ > + unsigned max_ver = 0; > + for (gphi_iterator gsi = gsi_start_phis (bb); > + !gsi_end_p (gsi); gsi_next (&gsi)) > + { > + gphi *phi = gsi.phi (); > + tree lhs = gimple_phi_result (phi); > + tree arg = gimple_phi_arg_def_from_edge (phi, e); > + if (m_names > + && TREE_CODE (arg) == SSA_NAME > + && TREE_CODE (TREE_TYPE (lhs)) == BITINT_TYPE > + && (bitint_precision_kind (TREE_TYPE (lhs)) > + > bitint_prec_middle) > + && bitmap_bit_p (m_names, SSA_NAME_VERSION (lhs)) > + && !bitmap_bit_p (m_names, SSA_NAME_VERSION (arg))) > + max_ver = MAX (max_ver, SSA_NAME_VERSION (arg)); > + } > + if (max_ver != 0) > + { > + if ((unsigned) m_map->var_partition->num_elements > + <= max_ver) > + { > + partition p = partition_new (max_ver + 1); > + partition o = m_map->var_partition; > + for (int e = 0; e < o->num_elements; ++e) > + { > + p->elements[e].class_element > + = o->elements[e].class_element; > + p->elements[e].class_count > + = o->elements[e].class_count; > + p->elements[e].next > + = &p->elements[0] + (o->elements[e].next > + - &o->elements[0]); > + } > + m_map->var_partition = p; > + partition_delete (o); > + } > + for (gphi_iterator gsi = gsi_start_phis (bb); > + !gsi_end_p (gsi); gsi_next (&gsi)) > + { > + gphi *phi = gsi.phi (); > + tree lhs = gimple_phi_result (phi); > + tree arg = gimple_phi_arg_def_from_edge (phi, e); > + if (m_names > + && TREE_CODE (arg) == SSA_NAME > + && TREE_CODE (TREE_TYPE (lhs)) == BITINT_TYPE > + && (bitint_precision_kind (TREE_TYPE (lhs)) > + > bitint_prec_middle) > + && bitmap_bit_p (m_names, SSA_NAME_VERSION (lhs)) > + && bitmap_set_bit (m_names, > + SSA_NAME_VERSION (arg))) > + partition_union (m_map->var_partition, > + SSA_NAME_VERSION (lhs), > + SSA_NAME_VERSION (arg)); > + } > + } > + returns_twice = 2; > + } > + gphi *phi = create_phi_node (make_ssa_name (TREE_TYPE (arg)), > + bb); > + add_phi_arg (phi, arg, e, UNKNOWN_LOCATION); > + bitmap_set_bit (m_preserved, SSA_NAME_VERSION (arg)); > + 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 (arg) = 1; > + add_phi_arg (phi, arg, ead, UNKNOWN_LOCATION); > + bitmap_set_bit (m_preserved, SSA_NAME_VERSION (arg)); > + arg = gimple_phi_result (phi); > + SSA_NAME_OCCURS_IN_ABNORMAL_PHI (arg) = 1; > + } > } > gimple_call_set_arg (stmt, i, arg); > - if (m_preserved == NULL) > - m_preserved = BITMAP_ALLOC (NULL); > bitmap_set_bit (m_preserved, SSA_NAME_VERSION (arg)); > } > tree lhs = gimple_call_lhs (stmt); > @@ -6870,6 +6955,9 @@ gimple_lower_bitint (void) > if (TREE_CODE (TREE_TYPE (lhs)) != BITINT_TYPE > || bitint_precision_kind (TREE_TYPE (lhs)) < bitint_prec_large) > continue; > + if (large_huge.m_preserved > + && bitmap_bit_p (large_huge.m_preserved, SSA_NAME_VERSION (lhs))) > + continue; > int p1 = var_to_partition (large_huge.m_map, lhs); > gcc_assert (large_huge.m_vars[p1] != NULL_TREE); > tree v1 = large_huge.m_vars[p1]; > --- gcc/testsuite/gcc.dg/bitint-100.c.jj 2024-03-13 15:21:02.313768843 +0100 > +++ gcc/testsuite/gcc.dg/bitint-100.c 2024-03-13 12:11:18.636036865 +0100 > @@ -0,0 +1,61 @@ > +/* PR tree-optimization/113466 */ > +/* { dg-do compile { target bitint575 } } */ > +/* { dg-options "-O2" } */ > + > +int foo (int); > + > +__attribute__((returns_twice, noipa)) _BitInt(325) > +bar (_BitInt(575) x) > +{ > + (void) x; > + return 0wb; > +} > + > +_BitInt(325) > +baz (_BitInt(575) y) > +{ > + foo (1); > + return bar (y); > +} > + > +_BitInt(325) > +qux (int x, _BitInt(575) y) > +{ > + if (x == 25) > + x = foo (2); > + else if (x == 42) > + x = foo (foo (3)); > + return bar (y); > +} > + > +void > +corge (int x, _BitInt(575) y, _BitInt(325) *z) > +{ > + void *q[] = { &&l1, &&l2, &&l3, &&l3 }; > + if (x == 25) > + { > + l1: > + x = foo (2); > + } > + else if (x == 42) > + { > + l2: > + x = foo (foo (3)); > + } > +l3: > + *z = bar (y); > + if (x < 4) > + goto *q[x & 3]; > +} > + > +_BitInt(325) > +freddy (int x, _BitInt(575) y) > +{ > + bar (y); > + ++y; > + if (x == 25) > + x = foo (2); > + else if (x == 42) > + x = foo (foo (3)); > + return bar (y); > +} > > Jakub > >
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? I can certainly try that and see what is shorter/more maintainable. Jakub
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. > I can certainly try that and see what is shorter/more maintainable. > > Jakub > >
--- gcc/gimple-lower-bitint.cc.jj 2024-03-13 13:03:08.120117081 +0100 +++ gcc/gimple-lower-bitint.cc 2024-03-13 15:05:54.830303524 +0100 @@ -5248,6 +5248,7 @@ bitint_large_huge::lower_call (tree obj, default: break; } + int 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); @@ -5255,6 +5256,8 @@ bitint_large_huge::lower_call (tree obj, || TREE_CODE (TREE_TYPE (arg)) != BITINT_TYPE || bitint_precision_kind (TREE_TYPE (arg)) <= bitint_prec_middle) continue; + if (m_preserved == NULL) + m_preserved = BITMAP_ALLOC (NULL); if (SSA_NAME_IS_DEFAULT_DEF (arg) && (!SSA_NAME_VAR (arg) || VAR_P (SSA_NAME_VAR (arg)))) { @@ -5270,11 +5273,93 @@ bitint_large_huge::lower_call (tree obj, v = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (arg), v); arg = make_ssa_name (TREE_TYPE (arg)); gimple *g = gimple_build_assign (arg, v); - gsi_insert_before (&gsi, g, GSI_SAME_STMT); + gsi_safe_insert_before (&gsi, g); + if (returns_twice) + { + basic_block bb = gimple_bb (stmt); + gcc_checking_assert (EDGE_COUNT (bb->preds) == 2); + edge 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)); + if (returns_twice == 1) + { + /* edge_before_returns_twice_call can use copy_ssa_name + for some PHIs, but in that case we need to put it + into the same partition as the copied SSA_NAME. */ + unsigned max_ver = 0; + for (gphi_iterator gsi = gsi_start_phis (bb); + !gsi_end_p (gsi); gsi_next (&gsi)) + { + gphi *phi = gsi.phi (); + tree lhs = gimple_phi_result (phi); + tree arg = gimple_phi_arg_def_from_edge (phi, e); + if (m_names + && TREE_CODE (arg) == SSA_NAME + && TREE_CODE (TREE_TYPE (lhs)) == BITINT_TYPE + && (bitint_precision_kind (TREE_TYPE (lhs)) + > bitint_prec_middle) + && bitmap_bit_p (m_names, SSA_NAME_VERSION (lhs)) + && !bitmap_bit_p (m_names, SSA_NAME_VERSION (arg))) + max_ver = MAX (max_ver, SSA_NAME_VERSION (arg)); + } + if (max_ver != 0) + { + if ((unsigned) m_map->var_partition->num_elements + <= max_ver) + { + partition p = partition_new (max_ver + 1); + partition o = m_map->var_partition; + for (int e = 0; e < o->num_elements; ++e) + { + p->elements[e].class_element + = o->elements[e].class_element; + p->elements[e].class_count + = o->elements[e].class_count; + p->elements[e].next + = &p->elements[0] + (o->elements[e].next + - &o->elements[0]); + } + m_map->var_partition = p; + partition_delete (o); + } + for (gphi_iterator gsi = gsi_start_phis (bb); + !gsi_end_p (gsi); gsi_next (&gsi)) + { + gphi *phi = gsi.phi (); + tree lhs = gimple_phi_result (phi); + tree arg = gimple_phi_arg_def_from_edge (phi, e); + if (m_names + && TREE_CODE (arg) == SSA_NAME + && TREE_CODE (TREE_TYPE (lhs)) == BITINT_TYPE + && (bitint_precision_kind (TREE_TYPE (lhs)) + > bitint_prec_middle) + && bitmap_bit_p (m_names, SSA_NAME_VERSION (lhs)) + && bitmap_set_bit (m_names, + SSA_NAME_VERSION (arg))) + partition_union (m_map->var_partition, + SSA_NAME_VERSION (lhs), + SSA_NAME_VERSION (arg)); + } + } + returns_twice = 2; + } + gphi *phi = create_phi_node (make_ssa_name (TREE_TYPE (arg)), + bb); + add_phi_arg (phi, arg, e, UNKNOWN_LOCATION); + bitmap_set_bit (m_preserved, SSA_NAME_VERSION (arg)); + 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 (arg) = 1; + add_phi_arg (phi, arg, ead, UNKNOWN_LOCATION); + bitmap_set_bit (m_preserved, SSA_NAME_VERSION (arg)); + arg = gimple_phi_result (phi); + SSA_NAME_OCCURS_IN_ABNORMAL_PHI (arg) = 1; + } } gimple_call_set_arg (stmt, i, arg); - if (m_preserved == NULL) - m_preserved = BITMAP_ALLOC (NULL); bitmap_set_bit (m_preserved, SSA_NAME_VERSION (arg)); } tree lhs = gimple_call_lhs (stmt); @@ -6870,6 +6955,9 @@ gimple_lower_bitint (void) if (TREE_CODE (TREE_TYPE (lhs)) != BITINT_TYPE || bitint_precision_kind (TREE_TYPE (lhs)) < bitint_prec_large) continue; + if (large_huge.m_preserved + && bitmap_bit_p (large_huge.m_preserved, SSA_NAME_VERSION (lhs))) + continue; int p1 = var_to_partition (large_huge.m_map, lhs); gcc_assert (large_huge.m_vars[p1] != NULL_TREE); tree v1 = large_huge.m_vars[p1]; --- gcc/testsuite/gcc.dg/bitint-100.c.jj 2024-03-13 15:21:02.313768843 +0100 +++ gcc/testsuite/gcc.dg/bitint-100.c 2024-03-13 12:11:18.636036865 +0100 @@ -0,0 +1,61 @@ +/* PR tree-optimization/113466 */ +/* { dg-do compile { target bitint575 } } */ +/* { dg-options "-O2" } */ + +int foo (int); + +__attribute__((returns_twice, noipa)) _BitInt(325) +bar (_BitInt(575) x) +{ + (void) x; + return 0wb; +} + +_BitInt(325) +baz (_BitInt(575) y) +{ + foo (1); + return bar (y); +} + +_BitInt(325) +qux (int x, _BitInt(575) y) +{ + if (x == 25) + x = foo (2); + else if (x == 42) + x = foo (foo (3)); + return bar (y); +} + +void +corge (int x, _BitInt(575) y, _BitInt(325) *z) +{ + void *q[] = { &&l1, &&l2, &&l3, &&l3 }; + if (x == 25) + { + l1: + x = foo (2); + } + else if (x == 42) + { + l2: + x = foo (foo (3)); + } +l3: + *z = bar (y); + if (x < 4) + goto *q[x & 3]; +} + +_BitInt(325) +freddy (int x, _BitInt(575) y) +{ + bar (y); + ++y; + if (x == 25) + x = foo (2); + else if (x == 42) + x = foo (foo (3)); + return bar (y); +}