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.
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
