On Thu, Feb 13, 2025 at 4:23 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
>
> A compare with zero may be taken as a sign bit test by
> fold_truth_andor_for_ifcombine, but the operand may be extended from a
> narrower field. If the operand was narrower, the bitsize will reflect
> the narrowing conversion, but if it was wider, we'll only know whether
> the field is sign- or zero-extended from unsignedp, but we won't know
> whether it needed to be extended, because arg will have changed to the
> narrower variable when we get to the point in which we can compute the
> arg width. If it's sign-extended, we're testing the right bit, but if
> it's zero-extended, there isn't any bit we can test.
>
> Instead of punting and leaving the foldable compare to be figured out
> by another pass, arrange for the sign bit resulting from the widening
> zero-extension to be taken as zero, so that the modified compare will
> yield the desired result.
>
> While at that, avoid swapping the right-hand compare operands when
> we've already determined that it was a signbit test: it no use to even
> try.
>
> Regstrapped on x86_64-linux-gnu. Ok to install?
OK.
Thanks,
Richard.
>
> for gcc/ChangeLog
>
> PR tree-optimization/118805
> * gimple-fold.cc (fold_truth_andor_for_combine): Detect and
> cope with zero-extension in signbit tests. Reject swapping
> right-compare operands if rsignbit.
>
> for gcc/testsuite/ChangeLog
>
> PR tree-optimization/118805
> * gcc.dg/field-merge-26.c: New.
> ---
> gcc/gimple-fold.cc | 22 +++++++++++++++++-----
> gcc/testsuite/gcc.dg/field-merge-26.c | 20 ++++++++++++++++++++
> 2 files changed, 37 insertions(+), 5 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/field-merge-26.c
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 29191685a43c5..0380c7af4c213 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -8090,14 +8090,16 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
>
> /* Prepare to turn compares of signed quantities with zero into sign-bit
> tests. We need not worry about *_reversep here for these compare
> - rewrites: loads will have already been reversed before compares. */
> - bool lsignbit = false, rsignbit = false;
> + rewrites: loads will have already been reversed before compares. Save
> the
> + precision, because [lr]l_arg may change and we won't be able to tell how
> + wide it was originally. */
> + unsigned lsignbit = 0, rsignbit = 0;
> if ((lcode == LT_EXPR || lcode == GE_EXPR)
> && integer_zerop (lr_arg)
> && INTEGRAL_TYPE_P (TREE_TYPE (ll_arg))
> && !TYPE_UNSIGNED (TREE_TYPE (ll_arg)))
> {
> - lsignbit = true;
> + lsignbit = TYPE_PRECISION (TREE_TYPE (ll_arg));
> lcode = (lcode == LT_EXPR ? NE_EXPR : EQ_EXPR);
> }
> /* Turn compares of unsigned quantities with powers of two into
> @@ -8130,7 +8132,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> && INTEGRAL_TYPE_P (TREE_TYPE (rl_arg))
> && !TYPE_UNSIGNED (TREE_TYPE (rl_arg)))
> {
> - rsignbit = true;
> + rsignbit = TYPE_PRECISION (TREE_TYPE (rl_arg));
> rcode = (rcode == LT_EXPR ? NE_EXPR : EQ_EXPR);
> }
> else if ((rcode == LT_EXPR || rcode == GE_EXPR)
> @@ -8204,7 +8206,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> || ! operand_equal_p (ll_inner, rl_inner, 0))
> {
> /* Try swapping the operands. */
> - if (ll_reversep != rr_reversep
> + if (ll_reversep != rr_reversep || rsignbit
> || !operand_equal_p (ll_inner, rr_inner, 0))
> return 0;
>
> @@ -8284,6 +8286,14 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> if (lsignbit)
> {
> wide_int sign = wi::mask (ll_bitsize - 1, true, ll_bitsize);
> + /* If ll_arg is zero-extended and we're testing the sign bit, we know
> + what the result should be. Shifting the sign bit out of sign will
> get
> + us to mask the entire field out, yielding zero, i.e., the sign bit of
> + the zero-extended value. We know the masked value is being compared
> + with zero, so the compare will get us the result we're looking
> + for: TRUE if EQ_EXPR, FALSE if NE_EXPR. */
> + if (lsignbit > ll_bitsize && ll_unsignedp)
> + sign <<= 1;
> if (!ll_and_mask.get_precision ())
> ll_and_mask = sign;
> else
> @@ -8303,6 +8313,8 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> if (rsignbit)
> {
> wide_int sign = wi::mask (rl_bitsize - 1, true, rl_bitsize);
> + if (rsignbit > rl_bitsize && ll_unsignedp)
> + sign <<= 1;
> if (!rl_and_mask.get_precision ())
> rl_and_mask = sign;
> else
> diff --git a/gcc/testsuite/gcc.dg/field-merge-26.c
> b/gcc/testsuite/gcc.dg/field-merge-26.c
> new file mode 100644
> index 0000000000000..96d7e7205c5f2
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/field-merge-26.c
> @@ -0,0 +1,20 @@
> +/* { dg-do run } */
> +/* { dg-options "-O1 -fno-tree-ccp -fno-tree-copy-prop -fno-tree-forwprop
> -fno-tree-fre" } */
> +
> +/* PR tree-optimization/118805 */
> +
> +/* Test that ifcombine doesn't get confused by tests for the sign bit of
> + extended values that would normally be folded before. */
> +
> +unsigned char a = 255;
> +int b;
> +int main() {
> + int c = 0;
> + if (c > a && a >= 255)
> + __builtin_abort ();
> + if (c <= a && a == 254)
> + __builtin_abort ();
> + if (a < c || a != 255)
> + __builtin_abort ();
> + return 0;
> +}
>
>
> --
> Alexandre Oliva, happy hacker https://FSFLA.org/blogs/lxo/
> Free Software Activist GNU Toolchain Engineer
> More tolerance and less prejudice are key for inclusion and diversity
> Excluding neuro-others for not behaving ""normal"" is *not* inclusive
