On Wed, 16 Apr 2025, Jakub Jelinek wrote:
> Hi!
>
> The following testcase is miscompiled, because we emit a CLOBBER in a place
> where it shouldn't be emitted.
> Before lowering we have:
> b_5 = 0;
> b.0_6 = b_5;
> b.1_1 = (unsigned _BitInt(129)) b.0_6;
> ...
> <retval> = b_5;
> The bitint coalescing assigns the same partition/underlying variable
> for both b_5 and b.0_6 (possible because there is a copy assignment)
> and of course a different one for b.1_1 (and other SSA_NAMEs in between).
> This is -O0 so stmts aren't DCEd and aren't propagated that much etc.
> It is -O0 so we also don't try to optimize and omit some names from m_names
> and handle multiple stmts at once, so the expansion emits essentially
> bitint.4 = {};
> bitint.4 = bitint.4;
> bitint.2 = cast of bitint.4;
> bitint.4 = CLOBBER;
> ...
> <retval> = bitint.4;
> and the CLOBBER is the problem because bitint.4 is still live afterwards.
> We emit the clobbers to improve code generation, but do it only for
> (initially) has_single_use SSA_NAMEs (remembered in m_single_use_names)
> being used, if they don't have the same partition on the lhs and a few
> other conditions.
> The problem above is that b.0_6 which is used in the cast has_single_use
> and so was in m_single_use_names bitmask and the lhs in that case is
> bitint.2, so a different partition. But there is gimple_assign_copy_p
> with SSA_NAME rhs1 and the partitioning special cases those and while
> b.0_6 is single use, b_5 has multiple uses. I believe this ought to be
> a problem solely in the case of such copy stmts and its special case
> by the partitioning, if instead of b.0_6 = b_5; there would be
> b.0_6 = b_5 + 1; or whatever other stmts that performs or may perform
> changes on the value, partitioning couldn't assign the same partition
> to b.0_6 and b_5 if b_5 is used later, it couldn't have two different
> (or potentially different) values in the same bitint.N var. With
> copy that is possible though.
>
> So the following patch fixes it by being more careful when we set
> m_single_use_names, don't set it if it is a has_single_use SSA_NAME
> but SSA_NAME_DEF_STMT of it is a copy stmt with SSA_NAME rhs1 and that
> rhs1 doesn't have single use, or has_single_use but SSA_NAME_DEF_STMT of it
> is a copy stmt etc.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> Just to make sure it doesn't change code generation too much, I've gathered
> statistics how many times
> if (m_first
> && m_single_use_names
> && m_vars[p] != m_lhs
> && m_after_stmt
> && bitmap_bit_p (m_single_use_names, SSA_NAME_VERSION (op)))
> {
> tree clobber = build_clobber (TREE_TYPE (m_vars[p]),
> CLOBBER_STORAGE_END);
> g = gimple_build_assign (m_vars[p], clobber);
> gimple_stmt_iterator gsi = gsi_for_stmt (m_after_stmt);
> gsi_insert_after (&gsi, g, GSI_SAME_STMT);
> }
> emits a clobber on
> make check-gcc GCC_TEST_RUN_EXPENSIVE=1
> RUNTESTFLAGS="--target_board=unix\{-m64,-m32\} GCC_TEST_RUN_EXPENSIVE=1
> dg.exp='*bitint* pr112673.c builtin-stdc-bit-*.c pr112566-2.c pr112511.c
> pr116588.c pr116003.c pr113693.c pr113602.c flex-array-counted-by-7.c'
> dg-torture.exp='*bitint* pr116480-2.c pr114312.c pr114121.c' dfp.exp=*bitint*
> i386.exp='pr118017.c pr117946.c apx-ndd-x32-2a.c'
> vect.exp='vect-early-break_99-pr113287.c' tree-ssa.exp=pr113735.c"
> and before this patch it was 41010 clobbers and after it is 40968,
> so difference is 42 clobbers, 0.1% fewer.
OK.
Richard.
> 2025-04-16 Jakub Jelinek <ja...@redhat.com>
>
> PR middle-end/119808
> * gimple-lower-bitint.cc (gimple_lower_bitint): Don't set
> m_single_use_names bits for SSA_NAMEs which have single use but
> their SSA_NAME_DEF_STMT is a copy from another SSA_NAME which doesn't
> have a single use, or single use which is such a copy etc.
>
> * gcc.dg/bitint-121.c: New test.
>
> --- gcc/gimple-lower-bitint.cc.jj 2025-04-12 13:13:47.543814860 +0200
> +++ gcc/gimple-lower-bitint.cc 2025-04-15 21:00:32.779348865 +0200
> @@ -6647,10 +6647,28 @@ gimple_lower_bitint (void)
> bitmap_set_bit (large_huge.m_names, SSA_NAME_VERSION (s));
> if (has_single_use (s))
> {
> - if (!large_huge.m_single_use_names)
> - large_huge.m_single_use_names = BITMAP_ALLOC (NULL);
> - bitmap_set_bit (large_huge.m_single_use_names,
> - SSA_NAME_VERSION (s));
> + tree s2 = s;
> + /* The coalescing hook special cases SSA_NAME copies.
> + Make sure not to mark in m_single_use_names single
> + use SSA_NAMEs copied from non-single use SSA_NAMEs. */
> + while (gimple_assign_copy_p (SSA_NAME_DEF_STMT (s2)))
> + {
> + s2 = gimple_assign_rhs1 (SSA_NAME_DEF_STMT (s2));
> + if (TREE_CODE (s2) != SSA_NAME)
> + break;
> + if (!has_single_use (s2))
> + {
> + s2 = NULL_TREE;
> + break;
> + }
> + }
> + if (s2)
> + {
> + if (!large_huge.m_single_use_names)
> + large_huge.m_single_use_names = BITMAP_ALLOC (NULL);
> + bitmap_set_bit (large_huge.m_single_use_names,
> + SSA_NAME_VERSION (s));
> + }
> }
> if (SSA_NAME_VAR (s)
> && ((TREE_CODE (SSA_NAME_VAR (s)) == PARM_DECL
> --- gcc/testsuite/gcc.dg/bitint-121.c.jj 2025-04-15 21:03:30.314955543
> +0200
> +++ gcc/testsuite/gcc.dg/bitint-121.c 2025-04-15 21:03:12.704192945 +0200
> @@ -0,0 +1,24 @@
> +/* PR middle-end/119808 */
> +/* { dg-do run { target { bitint && fstack_protector } } } */
> +/* { dg-options "-O0 -ftree-coalesce-vars -fstack-protector-strong" } */
> +
> +#if __BITINT_MAXWIDTH__ >= 129
> +_BitInt(129)
> +foo ()
> +{
> + _BitInt(129) b = 0;
> + _BitInt(8) a
> + =__builtin_stdc_rotate_right (0x8c82111b5d2d37c57e9ada7213ed95a49uwb, b);
> + return b;
> +}
> +#endif
> +
> +int
> +main ()
> +{
> +#if __BITINT_MAXWIDTH__ >= 129
> + _BitInt(129) x = foo ();
> + if (x)
> + __builtin_abort ();
> +#endif
> +}
>
> 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)
