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 <ja...@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
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH,
Frankenstrasse 146, 90461 Nuernberg, Germany;
GF: Ivo Totev, Andrew McDonald, Werner Knoblich; (HRB 36809, AG Nuernberg)
