On Fri, 23 Feb 2024, Jakub Jelinek wrote:
> Hi!
>
> The following testcases show 2 bugs in the .{ADD,SUB}_OVERFLOW lowering,
> both related to storing of the REALPART_EXPR part of the result.
> On the first testcase prec is 255, prec_limbs is 4 and for the second limb
> in the loop the store of the REALPART_EXPR of .USUBC (_30) is stored through:
> if (_27 <= 3)
> goto <bb 12>; [80.00%]
> else
> goto <bb 15>; [20.00%]
>
> <bb 12> [local count: 1073741824]:
> if (_27 < 3)
> goto <bb 14>; [80.00%]
> else
> goto <bb 13>; [20.00%]
>
> <bb 13> [local count: 1073741824]:
> bitint.3[_27] = _30;
> goto <bb 15>; [100.00%]
>
> <bb 14> [local count: 858993464]:
> MEM[(unsigned long *)&bitint.3 + 24B] = _30;
>
> <bb 15> [local count: 1073741824]:
> The first check is right, as prec_limbs is 4, we don't want to store
> bitint.3[4] or above at all, those limbs are just computed for the overflow
> checking and nothing else, so _27 > 4 leads to no store.
> But the other condition is exact opposite of what should be done, if
> the current index of the second limb (_27) is < 3, then it should
> bitint.3[_27] = _30;
> and if it is == 3, it should
> MEM[(unsigned long *)&bitint.3 + 24B] = _30;
> and (especially important for the targets which would bitinfo.extended = 1)
> should actually in this case zero extend it from the 63 bits to 64, that is
> the handling of the partial limb. The if_then_if_then_else helper if
> there are 2 conditions sets m_gsi to be at the start of the
> edge_true_false->dest bb, i.e. when the first condition is true and second
> false, and that is where we store the SSA_NAME indexed limb store, so the
> condition needs to be reversed.
>
> The following patch does that and adds the cast as well, the usual
> assumption that already handle_operand has the partial limb type doesn't
> have to be true here, because the source operand could have much larger
> precision than the REALPART_EXPR of the lhs.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
OK.
Richard.
> 2024-02-23 Jakub Jelinek <ja...@redhat.com>
>
> PR tree-optimization/114040
> * gimple-lower-bitint.cc (bitint_large_huge::lower_addsub_overflow):
> Use EQ_EXPR rather than LT_EXPR for g2 condition and change its
> probability from likely to unlikely. When handling the true true
> store, first cast to limb_access_type and then to l's type.
>
> * gcc.dg/torture/bitint-60.c: New test.
> * gcc.dg/torture/bitint-61.c: New test.
>
> --- gcc/gimple-lower-bitint.cc.jj 2024-02-22 10:13:54.123058152 +0100
> +++ gcc/gimple-lower-bitint.cc 2024-02-22 14:51:01.655335350 +0100
> @@ -4255,12 +4255,12 @@ bitint_large_huge::lower_addsub_overflow
> NULL_TREE, NULL_TREE);
> gimple *g2 = NULL;
> if (!single_comparison)
> - g2 = gimple_build_cond (LT_EXPR, idx,
> + g2 = gimple_build_cond (EQ_EXPR, idx,
> size_int (prec_limbs - 1),
> NULL_TREE, NULL_TREE);
> edge edge_true_true, edge_true_false, edge_false;
> if_then_if_then_else (g, g2, profile_probability::likely (),
> - profile_probability::likely (),
> + profile_probability::unlikely (),
> edge_true_true, edge_true_false,
> edge_false);
> tree l = limb_access (type, var ? var : obj, idx, true);
> @@ -4269,8 +4269,11 @@ bitint_large_huge::lower_addsub_overflow
> if (!single_comparison)
> {
> m_gsi = gsi_after_labels (edge_true_true->src);
> - l = limb_access (type, var ? var : obj,
> - size_int (prec_limbs - 1), true);
> + tree plm1idx = size_int (prec_limbs - 1);
> + tree plm1type = limb_access_type (type, plm1idx);
> + l = limb_access (type, var ? var : obj, plm1idx, true);
> + if (!useless_type_conversion_p (plm1type, TREE_TYPE (rhs)))
> + rhs = add_cast (plm1type, rhs);
> if (!useless_type_conversion_p (TREE_TYPE (l),
> TREE_TYPE (rhs)))
> rhs = add_cast (TREE_TYPE (l), rhs);
> --- gcc/testsuite/gcc.dg/torture/bitint-60.c.jj 2024-02-22
> 14:54:52.996121115 +0100
> +++ gcc/testsuite/gcc.dg/torture/bitint-60.c 2024-02-22 14:54:42.656264784
> +0100
> @@ -0,0 +1,24 @@
> +/* PR tree-optimization/114040 */
> +/* { dg-do run { target bitint } } */
> +/* { dg-options "-std=c23 -pedantic-errors" } */
> +/* { dg-skip-if "" { ! run_expensive_tests } { "*" } { "-O0" "-O2" } } */
> +/* { dg-skip-if "" { ! run_expensive_tests } { "-flto" } { "" } } */
> +
> +#if __BITINT_MAXWIDTH__ >= 8671
> +__attribute__((noipa)) unsigned
> +foo (unsigned _BitInt(8671) x, unsigned y, unsigned _BitInt(512) z)
> +{
> + unsigned _BitInt (8671) r
> + = x * __builtin_sub_overflow_p (y * z, 0, (unsigned _BitInt(255)) 0);
> + return r;
> +}
> +#endif
> +
> +int
> +main ()
> +{
> +#if __BITINT_MAXWIDTH__ >= 8671
> + if (foo (1, 1, 0xfffa46471e7c2dd60000000000000000wb))
> + __builtin_abort ();
> +#endif
> +}
> --- gcc/testsuite/gcc.dg/torture/bitint-61.c.jj 2024-02-22
> 15:19:24.562704298 +0100
> +++ gcc/testsuite/gcc.dg/torture/bitint-61.c 2024-02-22 15:19:57.964241439
> +0100
> @@ -0,0 +1,36 @@
> +/* PR tree-optimization/114040 */
> +/* { dg-do run { target { bitint && int128 } } } */
> +/* { dg-options "-std=c23" } */
> +/* { dg-skip-if "" { ! run_expensive_tests } { "*" } { "-O0" "-O2" } } */
> +/* { dg-skip-if "" { ! run_expensive_tests } { "-flto" } { "" } } */
> +
> +unsigned a;
> +signed char b;
> +short c;
> +long d;
> +__int128 e;
> +int f;
> +
> +#if __BITINT_MAXWIDTH__ >= 511
> +__attribute__((noinline)) void
> +foo (_BitInt(3) x, unsigned _BitInt(511) y, unsigned *z)
> +{
> + int g = __builtin_sub_overflow_p (y ^ x, 0, (unsigned _BitInt(255)) 0);
> + unsigned h = y + e, i = h + d;
> + unsigned _BitInt(2) j = i + g;
> + unsigned k = j + c;
> + unsigned l = k + a + f + b;
> + *z = l;
> +}
> +#endif
> +
> +int
> +main ()
> +{
> +#if __BITINT_MAXWIDTH__ >= 511
> + unsigned x;
> + foo (0, 0x81e4a5fa7c408f370000000000000000uwb, &x);
> + 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)
