> Am 11.04.2025 um 21:06 schrieb Jakub Jelinek <ja...@redhat.com>:
>
> Hi!
>
> The following patch is miscompiled, because during the limited
> SSA name coalescing the bitintlower pass does we incorrectly don't
> register a conflict.
> This is on
> <bb 4> [local count: 1073741824]:
> # b_17 = PHI <b_19(3), 8(2)>
> g.4_13 = g;
> _14 = g.4_13 >> 50;
> _15 = (unsigned int) _14;
> _21 = b_17;
> _16 = (unsigned int) _21;
> s_22 = _15 + _16;
> return s_22;
> basic block where in the map->bitint bitmap we track 14, 17 and 19.
> The build_bitint_stmt_ssa_conflicts "hook" has special code where
> it tracks uses at the final statements of mergeable operations, so
> e.g. the
> _16 = (unsigned int) _21;
> statement is considered to be use of b_17 because _21 is not in
> map->bitmap (or large_huge.m_names), i.e. is mergeable.
> The problem is that build_ssa_conflict_graph has special code to handle
> SSA_NAME copies and _21 = b_17; is gimple_assign_copy_p. In such cases
> it calls live_track_clear_var on the rhs1. The problem is that
> on the above bb, after we note in the _16 = (unsigned int) _21;
> stmt we need b_17 the generic code makes us forget that because
> of the copy statement, and then build_bitint_stmt_ssa_conflicts
> ignores it completely (because _21 is large/huge bitint and is
> not in map->bitint, so assumed to be handled by a later stmt in the
> bb, for backwards walk like this before this one).
> As the b_17 use is ignored, the coalescing thinks it can put
> all of b_17, b_19 and _14 into the same partition, which is wrong,
> while we can and should coalesce b_17 and b_19, _14 needs to be a different
> temporary because b_17 is set before and used after _14 has been written.
>
> The following patch fixes it by handling gimple_assign_copy_p in two
> separate spots, move the generic coalesce handling of it after
> build_ssa_conflict_graph (where build_ssa_conflict_graph handling
> doesn't fall through to that, it does continue after the call) and
> inside of build_ssa_conflict_graph it performs it too, but only if
> the lhs is not mergeable large/huge bitint.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
Ok
Richard
> 2025-04-11 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/119722
> * gimple-lower-bitint.h (build_bitint_stmt_ssa_conflicts): Add
> CLEAR argument.
> * gimple-lower-bitint.cc (build_bitint_stmt_ssa_conflicts): Add
> CLEAR argument. Call clear on gimple_assign_copy_p rhs1 if lhs
> is large/huge bitint unless lhs is not in names.
> * tree-ssa-coalesce.cc (build_ssa_conflict_graph): Adjust
> build_bitint_stmt_ssa_conflicts caller. Move gimple_assign_copy_p
> handling to after the build_bitint_stmt_ssa_conflicts call.
>
> * gcc.dg/torture/bitint-77.c: New test.
>
> --- gcc/gimple-lower-bitint.h.jj 2025-04-08 14:08:51.644276395 +0200
> +++ gcc/gimple-lower-bitint.h 2025-04-11 13:03:12.231981283 +0200
> @@ -26,6 +26,7 @@ extern void build_bitint_stmt_ssa_confli
> ssa_conflicts *, bitmap,
> void (*) (live_track *, tree,
> ssa_conflicts *),
> + void (*) (live_track *, tree),
> void (*) (live_track *, tree));
>
> #endif /* GCC_GIMPLE_LOWER_BITINT_H */
> --- gcc/gimple-lower-bitint.cc.jj 2025-04-11 08:27:47.841168976 +0200
> +++ gcc/gimple-lower-bitint.cc 2025-04-11 13:07:14.396626536 +0200
> @@ -5919,7 +5919,8 @@ build_bitint_stmt_ssa_conflicts (gimple
> ssa_conflicts *graph, bitmap names,
> void (*def) (live_track *, tree,
> ssa_conflicts *),
> - void (*use) (live_track *, tree))
> + void (*use) (live_track *, tree),
> + void (*clear) (live_track *, tree))
> {
> bool muldiv_p = false;
> tree lhs = NULL_TREE;
> @@ -5936,6 +5937,25 @@ build_bitint_stmt_ssa_conflicts (gimple
> {
> if (!bitmap_bit_p (names, SSA_NAME_VERSION (lhs)))
> return;
> +
> + /* A copy between 2 partitions does not introduce an interference
> + by itself. If they did, you would never be able to coalesce
> + two things which are copied. If the two variables really do
> + conflict, they will conflict elsewhere in the program.
> +
> + This is handled by simply removing the SRC of the copy from
> + the live list, and processing the stmt normally.
> +
> + Don't do this if lhs is not in names though, in such cases
> + it is actually used at some point later in the basic
> + block. */
> + if (gimple_assign_copy_p (stmt))
> + {
> + tree rhs1 = gimple_assign_rhs1 (stmt);
> + if (TREE_CODE (rhs1) == SSA_NAME)
> + clear (live, rhs1);
> + }
> +
> switch (gimple_assign_rhs_code (stmt))
> {
> case MULT_EXPR:
> --- gcc/tree-ssa-coalesce.cc.jj 2025-04-08 14:09:29.282752418 +0200
> +++ gcc/tree-ssa-coalesce.cc 2025-04-11 13:00:13.120462546 +0200
> @@ -896,6 +896,18 @@ build_ssa_conflict_graph (tree_live_info
> tree var;
> gimple *stmt = gsi_stmt (gsi);
>
> + if (is_gimple_debug (stmt))
> + continue;
> +
> + if (map->bitint)
> + {
> + build_bitint_stmt_ssa_conflicts (stmt, live, graph, map->bitint,
> + live_track_process_def,
> + live_track_process_use,
> + live_track_clear_var);
> + continue;
> + }
> +
> /* A copy between 2 partitions does not introduce an interference
> by itself. If they did, you would never be able to coalesce
> two things which are copied. If the two variables really do
> @@ -912,16 +924,6 @@ build_ssa_conflict_graph (tree_live_info
> && TREE_CODE (rhs1) == SSA_NAME)
> live_track_clear_var (live, rhs1);
> }
> - else if (is_gimple_debug (stmt))
> - continue;
> -
> - if (map->bitint)
> - {
> - build_bitint_stmt_ssa_conflicts (stmt, live, graph, map->bitint,
> - live_track_process_def,
> - live_track_process_use);
> - continue;
> - }
>
> /* For stmts with more than one SSA_NAME definition pretend all the
> SSA_NAME outputs but the first one are live at this point, so
> --- gcc/testsuite/gcc.dg/torture/bitint-77.c.jj 2025-04-11
> 13:13:49.286137937 +0200
> +++ gcc/testsuite/gcc.dg/torture/bitint-77.c 2025-04-11 13:13:21.815523489
> +0200
> @@ -0,0 +1,26 @@
> +/* PR tree-optimization/119722 */
> +/* { dg-do run { target bitint } } */
> +/* { dg-options "-O2 -fno-tree-forwprop -fno-tree-copy-prop -fno-tree-fre" }
> */
> +
> +#if __BITINT_MAXWIDTH__ >= 33300
> +unsigned _BitInt(33300) g;
> +
> +unsigned
> +foo (long c)
> +{
> + unsigned _BitInt(33300) b
> + = __builtin_stdc_rotate_left ((unsigned _BitInt(13)) 8, c);
> + return ((unsigned _BitInt(50)) (g >> 50)
> + + ({ unsigned _BitInt(300) unused; b; }));
> +}
> +#endif
> +
> +int
> +main ()
> +{
> +#if __BITINT_MAXWIDTH__ >= 33300
> + unsigned x = foo (0);
> + if (x != 8)
> + __builtin_abort ();
> +#endif
> +}
>
> Jakub
>
