On Thu, Dec 5, 2024 at 1:39 PM Alexandre Oliva <ol...@adacore.com> wrote:
>
> Hi,
>
> Thanks for the review.
>
> I hadn't seen your response yet, and for days I have been just about to
> send v3 with some incremental changes.
>
> Here they are, for the record. They fix a bug I had introduced while
> converting constants and masks to wide_int, simplifies some internal
> APIs, reworks some variable naming, ensures right-hand masks are
> reapplied where in some cases they weren't, drops other unnecessary
> masks, and simplifies the handling of mask and constant widths to
> disregard signedness, since at that point we've already checked
> signedness and we're interested in known bit ranges, so sign extension
> is just not necessary.
>
> I'll hold off from posting the v3 I was just about to post, implement
> the changes recommended in your response and then post another
> incremental, for the record, and the resulting v3.
The incremental changes look good, I have nothing to add there
to the review of v2.
Thanks,
Richard.
> Thanks,
>
>
> diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
> index 149df985bee41..2c33cdfb1b29a 100644
> --- a/gcc/gimple-fold.cc
> +++ b/gcc/gimple-fold.cc
> @@ -7446,9 +7446,6 @@ maybe_fold_comparisons_from_match_pd (tree type, enum
> tree_code code,
> *PBITSIZE is set to the number of bits in the reference, *PBITPOS is
> set to the starting bit number.
>
> - If the innermost field can be completely contained in a mode-sized
> - unit, *PMODE is set to that mode. Otherwise, it is set to VOIDmode.
> -
> *PVOLATILEP is set to 1 if the any expression encountered is volatile;
> otherwise it is not changed.
>
> @@ -7456,10 +7453,8 @@ maybe_fold_comparisons_from_match_pd (tree type, enum
> tree_code code,
>
> *PREVERSEP is set to the storage order of the field.
>
> - *PMASK is set to the mask used. This is either contained in a
> - BIT_AND_EXPR or derived from the width of the field.
> -
> *PAND_MASK is set to the mask found in a BIT_AND_EXPR, if any.
> + If PAND_MASK *is NULL, BIT_AND_EXPR is not recognized.
>
> *XOR_P is to be FALSE if EXP might be a XOR used in a compare, in which
> case, if XOR_CMP_OP is a zero constant, it will be overridden with *PEXP,
> @@ -7478,10 +7473,9 @@ maybe_fold_comparisons_from_match_pd (tree type, enum
> tree_code code,
>
> static tree
> decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
> - HOST_WIDE_INT *pbitpos, machine_mode *pmode,
> + HOST_WIDE_INT *pbitpos,
> bool *punsignedp, bool *preversep, bool *pvolatilep,
> - wide_int *pmask, wide_int *pand_mask,
> - bool *xor_p, tree *xor_cmp_op,
> + wide_int *pand_mask, bool *xor_p, tree *xor_cmp_op,
> gimple **load, location_t loc[4])
> {
> /* These are from match.pd. */
> @@ -7494,10 +7488,9 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> tree outer_type = 0;
> wide_int and_mask;
> tree inner, offset;
> - unsigned int precision;
> int shiftrt = 0;
> - wide_int mask;
> tree res_ops[2];
> + machine_mode mode;
>
> *load = NULL;
>
> @@ -7522,7 +7515,7 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> }
>
> /* Recognize and save a masking operation. */
> - if (gimple_bit_and_cst (exp, res_ops, follow_all_ssa_edges))
> + if (pand_mask && gimple_bit_and_cst (exp, res_ops, follow_all_ssa_edges))
> {
> loc[1] = gimple_location (SSA_NAME_DEF_STMT (exp));
> exp = res_ops[0];
> @@ -7600,11 +7593,10 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> poly_int64 poly_bitsize, poly_bitpos;
> int unsignedp, reversep = *preversep, volatilep = *pvolatilep;
> inner = get_inner_reference (exp, &poly_bitsize, &poly_bitpos, &offset,
> - pmode, &unsignedp, &reversep, &volatilep);
> + &mode, &unsignedp, &reversep, &volatilep);
>
> HOST_WIDE_INT bs, bp;
> - if ((inner == exp && !and_mask.get_precision ())
> - || !poly_bitsize.is_constant (&bs)
> + if (!poly_bitsize.is_constant (&bs)
> || !poly_bitpos.is_constant (&bp)
> || bs <= shiftrt
> || offset != 0
> @@ -7646,17 +7638,13 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT
> *pbitsize,
> if (outer_type && *pbitsize == TYPE_PRECISION (outer_type))
> *punsignedp = TYPE_UNSIGNED (outer_type);
>
> - /* Compute the mask to access the bitfield. */
> - precision = *pbitsize;
> -
> - mask = wi::mask (*pbitsize, false, precision);
> -
> - /* Merge it with the mask we found in the BIT_AND_EXPR, if any. */
> + /* Make the mask the expected width. */
> if (and_mask.get_precision () != 0)
> - mask &= wide_int::from (and_mask, precision, UNSIGNED);
> + and_mask = wide_int::from (and_mask, *pbitsize, UNSIGNED);
> +
> + if (pand_mask)
> + *pand_mask = and_mask;
>
> - *pmask = mask;
> - *pand_mask = and_mask;
> return inner;
> }
>
> @@ -7913,9 +7901,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> HOST_WIDE_INT rnbitsize, rnbitpos, rnprec;
> bool ll_unsignedp, lr_unsignedp, rl_unsignedp, rr_unsignedp;
> bool ll_reversep, lr_reversep, rl_reversep, rr_reversep;
> - machine_mode ll_mode, lr_mode, rl_mode, rr_mode;
> scalar_int_mode lnmode, lnmode2, rnmode;
> - wide_int ll_mask, lr_mask, rl_mask, rr_mask;
> wide_int ll_and_mask, lr_and_mask, rl_and_mask, rr_and_mask;
> wide_int l_const, r_const;
> tree lntype, rntype, result;
> @@ -7987,25 +7973,21 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> ll_reversep = lr_reversep = rl_reversep = rr_reversep = 0;
> volatilep = 0;
> bool l_xor = false, r_xor = false;
> - ll_inner = decode_field_reference (&ll_arg,
> - &ll_bitsize, &ll_bitpos, &ll_mode,
> + ll_inner = decode_field_reference (&ll_arg, &ll_bitsize, &ll_bitpos,
> &ll_unsignedp, &ll_reversep, &volatilep,
> - &ll_mask, &ll_and_mask, &l_xor, &lr_arg,
> + &ll_and_mask, &l_xor, &lr_arg,
> &ll_load, ll_loc);
> - lr_inner = decode_field_reference (&lr_arg,
> - &lr_bitsize, &lr_bitpos, &lr_mode,
> + lr_inner = decode_field_reference (&lr_arg, &lr_bitsize, &lr_bitpos,
> &lr_unsignedp, &lr_reversep, &volatilep,
> - &lr_mask, &lr_and_mask, &l_xor, 0,
> + &lr_and_mask, &l_xor, 0,
> &lr_load, lr_loc);
> - rl_inner = decode_field_reference (&rl_arg,
> - &rl_bitsize, &rl_bitpos, &rl_mode,
> + rl_inner = decode_field_reference (&rl_arg, &rl_bitsize, &rl_bitpos,
> &rl_unsignedp, &rl_reversep, &volatilep,
> - &rl_mask, &rl_and_mask, &r_xor, &rr_arg,
> + &rl_and_mask, &r_xor, &rr_arg,
> &rl_load, rl_loc);
> - rr_inner = decode_field_reference (&rr_arg,
> - &rr_bitsize, &rr_bitpos, &rr_mode,
> + rr_inner = decode_field_reference (&rr_arg, &rr_bitsize, &rr_bitpos,
> &rr_unsignedp, &rr_reversep, &volatilep,
> - &rr_mask, &rr_and_mask, &r_xor, 0,
> + &rr_and_mask, &r_xor, 0,
> &rr_load, rr_loc);
>
> /* It must be true that the inner operation on the lhs of each
> @@ -8024,7 +8006,15 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> && TREE_CODE (rr_arg) == INTEGER_CST)
> {
> l_const = wi::to_wide (lr_arg);
> + /* We don't expect masks on constants, but if there are any, apply
> + them now. */
> + if (lr_and_mask.get_precision ())
> + l_const &= wide_int::from (lr_and_mask,
> + l_const.get_precision (), UNSIGNED);
> r_const = wi::to_wide (rr_arg);
> + if (rr_and_mask.get_precision ())
> + r_const &= wide_int::from (rr_and_mask,
> + r_const.get_precision (), UNSIGNED);
> lr_reversep = ll_reversep;
> }
> else if (lr_reversep != rr_reversep
> @@ -8038,12 +8028,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
>
> if (lsignbit)
> {
> - wide_int sign = wi::mask (ll_bitsize - 1, true,
> - TYPE_PRECISION (TREE_TYPE (ll_arg)));
> - if (!ll_mask.get_precision ())
> - ll_mask = sign;
> - else
> - ll_mask &= sign;
> + wide_int sign = wi::mask (ll_bitsize - 1, true, ll_bitsize);
> if (!ll_and_mask.get_precision ())
> ll_and_mask = sign;
> else
> @@ -8052,12 +8037,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
>
> if (rsignbit)
> {
> - wide_int sign = wi::mask (rl_bitsize - 1, true,
> - TYPE_PRECISION (TREE_TYPE (rl_arg)));
> - if (!rl_mask.get_precision ())
> - rl_mask = sign;
> - else
> - rl_mask &= sign;
> + wide_int sign = wi::mask (rl_bitsize - 1, true, rl_bitsize);
> if (!rl_and_mask.get_precision ())
> rl_and_mask = sign;
> else
> @@ -8073,13 +8053,14 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> {
> if (l_const.get_precision ()
> && l_const == 0
> - && wi::popcount (ll_mask) == 1)
> + && ll_and_mask.get_precision ()
> + && wi::popcount (ll_and_mask) == 1)
> {
> /* Make the left operand unsigned, since we are only interested
> in the value of one bit. Otherwise we are doing the wrong
> thing below. */
> ll_unsignedp = 1;
> - l_const = ll_mask;
> + l_const = ll_and_mask;
> }
> else
> return 0;
> @@ -8090,10 +8071,11 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> {
> if (r_const.get_precision ()
> && r_const == 0
> - && wi::popcount (rl_mask) == 1)
> + && rl_and_mask.get_precision ()
> + && wi::popcount (rl_and_mask) == 1)
> {
> rl_unsignedp = 1;
> - r_const = rl_mask;
> + r_const = rl_and_mask;
> }
> else
> return 0;
> @@ -8231,23 +8213,27 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> }
>
> /* Adjust masks to match the positions in the combined lntype. */
> - ll_mask = wi::lshift (wide_int::from (ll_mask, lnprec, UNSIGNED),
> - xll_bitpos);
> - rl_mask = wi::lshift (wide_int::from (rl_mask, lnprec, UNSIGNED),
> - xrl_bitpos);
> + wide_int ll_mask, rl_mask, r_mask;
> + if (ll_and_mask.get_precision ())
> + ll_mask = wi::lshift (wide_int::from (ll_and_mask, lnprec, UNSIGNED),
> + xll_bitpos);
> + else
> + ll_mask = wi::shifted_mask (xll_bitpos, ll_bitsize, false, lnprec);
> + if (rl_and_mask.get_precision ())
> + rl_mask = wi::lshift (wide_int::from (rl_and_mask, lnprec, UNSIGNED),
> + xrl_bitpos);
> + else
> + rl_mask = wi::shifted_mask (xrl_bitpos, rl_bitsize, false, lnprec);
>
> /* Adjust right-hand constants in both original comparisons to match width
> and bit position. */
> if (l_const.get_precision ())
> {
> - l_const = wide_int::from (l_const, lnprec,
> - TYPE_SIGN (TREE_TYPE (lr_arg)));
> - if (!TYPE_UNSIGNED (TREE_TYPE (lr_arg)))
> - {
> - l_const = wi::zext (l_const, TYPE_PRECISION (TREE_TYPE (lr_arg)));
> - if (ll_and_mask.get_precision ())
> - l_const &= wide_int::from (ll_and_mask, lnprec, UNSIGNED);
> - }
> + /* We're doing bitwise equality tests, so don't bother with sign
> + extensions. */
> + l_const = wide_int::from (l_const, lnprec, UNSIGNED);
> + if (ll_and_mask.get_precision ())
> + l_const &= wide_int::from (ll_and_mask, lnprec, UNSIGNED);
> l_const <<= xll_bitpos;
> if ((l_const & ~ll_mask) != 0)
> {
> @@ -8260,14 +8246,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> again. */
> gcc_checking_assert (r_const.get_precision ());
>
> - r_const = wide_int::from (r_const, lnprec,
> - TYPE_SIGN (TREE_TYPE (rr_arg)));
> - if (!TYPE_UNSIGNED (TREE_TYPE (rr_arg)))
> - {
> - r_const = wi::zext (r_const, TYPE_PRECISION (TREE_TYPE (rr_arg)));
> - if (rl_and_mask.get_precision ())
> - r_const &= wide_int::from (rl_and_mask, lnprec, UNSIGNED);
> - }
> + r_const = wide_int::from (r_const, lnprec, UNSIGNED);
> + if (rl_and_mask.get_precision ())
> + r_const &= wide_int::from (rl_and_mask, lnprec, UNSIGNED);
> r_const <<= xrl_bitpos;
> if ((r_const & ~rl_mask) != 0)
> {
> @@ -8306,7 +8287,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> lr_reversep);
>
> /* No masking needed, we know the full constants. */
> - lr_mask = wi::mask (0, true, lnprec);
> + r_mask = wi::mask (0, true, lnprec);
>
> /* If the compiler thinks this is used uninitialized below, it's
> because it can't realize that parts can only be 2 when
> @@ -8391,10 +8372,18 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> }
>
> /* Adjust the masks to match the combined type, and combine them. */
> - lr_mask = wide_int::from (lr_mask, rnprec, UNSIGNED) << xlr_bitpos;
> - rr_mask = wide_int::from (rr_mask, rnprec, UNSIGNED) << xrr_bitpos;
> -
> - lr_mask |= rr_mask;
> + wide_int lr_mask, rr_mask;
> + if (lr_and_mask.get_precision ())
> + lr_mask = wi::lshift (wide_int::from (lr_and_mask, rnprec, UNSIGNED),
> + xlr_bitpos);
> + else
> + lr_mask = wi::shifted_mask (xlr_bitpos, lr_bitsize, false, rnprec);
> + if (rl_and_mask.get_precision ())
> + rr_mask = wi::lshift (wide_int::from (rr_and_mask, rnprec, UNSIGNED),
> + xrr_bitpos);
> + else
> + rr_mask = wi::shifted_mask (xrr_bitpos, rr_bitsize, false, rnprec);
> + r_mask = lr_mask | rr_mask;
>
> /* Load the right-hand operand of the combined compare. */
> toshift[1][0] = MIN (xlr_bitpos, xrr_bitpos);
> @@ -8431,7 +8420,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> }
>
> /* Now issue the loads for the left-hand combined operand/s. */
> - ll_mask |= rl_mask;
> + wide_int l_mask = ll_mask | rl_mask;
> toshift[0][0] = MIN (xll_bitpos, xrl_bitpos);
> shifted[0][0] = 0;
>
> @@ -8467,7 +8456,7 @@ fold_truth_andor_for_ifcombine (enum tree_code code,
> tree truth_type,
> for (int i = 0; i < parts; i++)
> {
> tree op[2] = { ld_arg[0][i], ld_arg[1][i] };
> - wide_int mask[2] = { ll_mask, lr_mask };
> + wide_int mask[2] = { l_mask, r_mask };
> location_t *locs[2] = { i ? rl_loc : ll_loc, i ? rr_loc : lr_loc };
>
> /* Figure out the masks, and unshare the original operands. */
> diff --git a/gcc/testsuite/gcc.dg/field-merge-12.c
> b/gcc/testsuite/gcc.dg/field-merge-12.c
> new file mode 100644
> index 0000000000000..7056eb607e904
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/field-merge-12.c
> @@ -0,0 +1,33 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2" } */
> +
> +/* Check that we don't crash when trying to handle masks that don't match the
> + width of the original type. */
> +
> +struct s {
> + long long q;
> +};
> +
> +struct s x1 = { 1 };
> +struct s xm1 = { -1 };
> +struct s x8 = { 8 };
> +struct s x0 = { 0 };
> +
> +bool f(struct s *p)
> +{
> + int q = (int)p->q;
> + return (q < 0) || (q & 7);
> +}
> +
> +int main ()
> +{
> + if (!f (&x1))
> + __builtin_abort ();
> + if (!f (&xm1))
> + __builtin_abort ();
> + if (f (&x8))
> + __builtin_abort ();
> + if (f (&x0))
> + __builtin_abort ();
> + return 0;
> +}
> diff --git a/gcc/testsuite/gcc.dg/field-merge-9.c
> b/gcc/testsuite/gcc.dg/field-merge-9.c
> index 25a8b1fa9b0ab..b9e08d8fa37d2 100644
> --- a/gcc/testsuite/gcc.dg/field-merge-9.c
> +++ b/gcc/testsuite/gcc.dg/field-merge-9.c
> @@ -1,5 +1,5 @@
> /* { dg-do run } */
> -/* { dg-options "-O" } */
> +/* { dg-options "-O -fdump-tree-ifcombine-details" } */
>
> /* Check that conversions and selections of similar bit ranges across
> different
> types don't prevent combination. */
> @@ -25,7 +25,7 @@ void f (void) {
> if (0
> || p.a != q.a
> || p.b[!le] != (unsigned char)q.b
> - || p.b[le] != (char)((q.b >> (__CHAR_BIT__ - 1)) >> 1)
> + || p.b[le] != (unsigned char)((q.b >> (__CHAR_BIT__ - 1)) >> 1)
> )
> __builtin_abort ();
> }
> @@ -34,3 +34,5 @@ int main () {
> f ();
> return 0;
> }
> +
> +/* { dg-final { scan-tree-dump-times "optimizing two comparisons" 2
> "ifcombine" } } */
>
>
> --
> 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
