[PATCH] [ifcombine] robustify decode_field_reference

[Alexandre Oliva <oliva@adacore.com>]

Arrange for decode_field_reference to use local variables throughout,
to modify the out parms only when we're about to return non-NULL, and
to drop the unused case of NULL pand_mask, that had a latent failure
to detect signbit masking.

Regstrapped on x86_64-linux-gnu along with the PR118456 patch.
Ok to install?


for  gcc/ChangeLog

	* gimple-fold.cc (decode_field_reference): Rebustify to set
	out parms only when returning non-NULL.
	(fold_truth_andor_for_ifcombine): Bail if
	decode_field_reference returns NULL.  Add complementary assert
	on r_const's not being set when l_const isn't.
---
 gcc/gimple-fold.cc |  155 +++++++++++++++++++++++++++-------------------------
 1 file changed, 80 insertions(+), 75 deletions(-)

diff --git a/gcc/gimple-fold.cc b/gcc/gimple-fold.cc
index 5b1fbe6db1df3..3c971a29ef045 100644
--- a/gcc/gimple-fold.cc
+++ b/gcc/gimple-fold.cc
@@ -7510,18 +7510,17 @@ gimple_binop_def_p (enum tree_code code, tree t, tree op[2])
    *PREVERSEP is set to the storage order 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.  If *PAND_MASK
-   is initially set to a mask with nonzero precision, that mask is
+   *PAND_MASK is initially set to a mask with nonzero precision, that mask is
    combined with the found mask, or adjusted in precision to match.
 
    *PSIGNBIT is set to TRUE if, before clipping to *PBITSIZE, the mask
    encompassed bits that corresponded to extensions of the sign bit.
 
-   *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,
-   *XOR_P will be set to TRUE, *XOR_PAND_MASK will be copied from *PAND_MASK,
-   and the left-hand operand of the XOR will be decoded.  If *XOR_P is TRUE,
-   XOR_CMP_OP and XOR_PAND_MASK are supposed to be NULL, and then the
+   *PXORP is to be FALSE if EXP might be a XOR used in a compare, in which
+   case, if PXOR_CMP_OP is a zero constant, it will be overridden with *PEXP,
+   *PXORP will be set to TRUE, *PXOR_AND_MASK will be copied from *PAND_MASK,
+   and the left-hand operand of the XOR will be decoded.  If *PXORP is TRUE,
+   PXOR_CMP_OP and PXOR_AND_MASK are supposed to be NULL, and then the
    right-hand operand of the XOR will be decoded.
 
    *LOAD is set to the load stmt of the innermost reference, if any,
@@ -7538,8 +7537,8 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
 			HOST_WIDE_INT *pbitpos,
 			bool *punsignedp, bool *preversep, bool *pvolatilep,
 			wide_int *pand_mask, bool *psignbit,
-			bool *xor_p, tree *xor_cmp_op, wide_int *xor_pand_mask,
-			gimple **load, location_t loc[4])
+			bool *pxorp, tree *pxor_cmp_op, wide_int *pxor_and_mask,
+			gimple **pload, location_t loc[4])
 {
   tree exp = *pexp;
   tree outer_type = 0;
@@ -7549,9 +7548,11 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
   tree res_ops[2];
   machine_mode mode;
   bool convert_before_shift = false;
-
-  *load = NULL;
-  *psignbit = false;
+  bool signbit = false;
+  bool xorp = false;
+  tree xor_cmp_op;
+  wide_int xor_and_mask;
+  gimple *load = NULL;
 
   /* All the optimizations using this function assume integer fields.
      There are problems with FP fields since the type_for_size call
@@ -7576,7 +7577,7 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
 
   /* Recognize and save a masking operation.  Combine it with an
      incoming mask.  */
-  if (pand_mask && gimple_binop_def_p (BIT_AND_EXPR, exp, res_ops)
+  if (gimple_binop_def_p (BIT_AND_EXPR, exp, res_ops)
       && TREE_CODE (res_ops[1]) == INTEGER_CST)
     {
       loc[1] = gimple_location (SSA_NAME_DEF_STMT (exp));
@@ -7596,29 +7597,29 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
 	    and_mask &= wide_int::from (*pand_mask, prec_op, UNSIGNED);
 	}
     }
-  else if (pand_mask)
+  else
     and_mask = *pand_mask;
 
   /* Turn (a ^ b) [!]= 0 into a [!]= b.  */
-  if (xor_p && gimple_binop_def_p (BIT_XOR_EXPR, exp, res_ops))
+  if (pxorp && gimple_binop_def_p (BIT_XOR_EXPR, exp, res_ops))
     {
       /* No location recorded for this one, it's entirely subsumed by the
 	 compare.  */
-      if (*xor_p)
+      if (*pxorp)
 	{
 	  exp = res_ops[1];
-	  gcc_checking_assert (!xor_cmp_op && !xor_pand_mask);
+	  gcc_checking_assert (!pxor_cmp_op && !pxor_and_mask);
 	}
-      else if (!xor_cmp_op)
+      else if (!pxor_cmp_op)
 	/* Not much we can do when xor appears in the right-hand compare
 	   operand.  */
 	return NULL_TREE;
-      else if (integer_zerop (*xor_cmp_op))
+      else if (integer_zerop (*pxor_cmp_op))
 	{
-	  *xor_p = true;
+	  xorp = true;
 	  exp = res_ops[0];
-	  *xor_cmp_op = *pexp;
-	  *xor_pand_mask = *pand_mask;
+	  xor_cmp_op = *pexp;
+	  xor_and_mask = *pand_mask;
 	}
     }
 
@@ -7646,12 +7647,12 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
   /* Yet another chance to drop conversions.  This one is allowed to
      match a converting load, subsuming the load identification block
      below.  */
-  if (!outer_type && gimple_convert_def_p (exp, res_ops, load))
+  if (!outer_type && gimple_convert_def_p (exp, res_ops, &load))
     {
       outer_type = TREE_TYPE (exp);
       loc[0] = gimple_location (SSA_NAME_DEF_STMT (exp));
-      if (*load)
-	loc[3] = gimple_location (*load);
+      if (load)
+	loc[3] = gimple_location (load);
       exp = res_ops[0];
       /* This looks backwards, but we're going back the def chain, so if we
 	 find the conversion here, after finding a shift, that's because the
@@ -7662,14 +7663,13 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
     }
 
   /* Identify the load, if there is one.  */
-  if (!(*load) && TREE_CODE (exp) == SSA_NAME
-      && !SSA_NAME_IS_DEFAULT_DEF (exp))
+  if (!load && TREE_CODE (exp) == SSA_NAME && !SSA_NAME_IS_DEFAULT_DEF (exp))
     {
       gimple *def = SSA_NAME_DEF_STMT (exp);
       if (gimple_assign_load_p (def))
 	{
 	  loc[3] = gimple_location (def);
-	  *load = def;
+	  load = def;
 	  exp = gimple_assign_rhs1 (def);
 	}
     }
@@ -7694,20 +7694,14 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
 	  && !type_has_mode_precision_p (TREE_TYPE (inner))))
     return NULL_TREE;
 
-  *pbitsize = bs;
-  *pbitpos = bp;
-  *punsignedp = unsignedp;
-  *preversep = reversep;
-  *pvolatilep = volatilep;
-
   /* Adjust shifts...  */
   if (convert_before_shift
-      && outer_type && *pbitsize > TYPE_PRECISION (outer_type))
+      && outer_type && bs > TYPE_PRECISION (outer_type))
     {
-      HOST_WIDE_INT excess = *pbitsize - TYPE_PRECISION (outer_type);
-      if (*preversep ? !BYTES_BIG_ENDIAN : BYTES_BIG_ENDIAN)
-	*pbitpos += excess;
-      *pbitsize -= excess;
+      HOST_WIDE_INT excess = bs - TYPE_PRECISION (outer_type);
+      if (reversep ? !BYTES_BIG_ENDIAN : BYTES_BIG_ENDIAN)
+	bp += excess;
+      bs -= excess;
     }
 
   if (shiftrt)
@@ -7720,49 +7714,57 @@ decode_field_reference (tree *pexp, HOST_WIDE_INT *pbitsize,
 	 shift after a change of signedness would make the extension
 	 non-uniform, and we can't deal with that (yet ???).  See
 	 gcc.dg/field-merge-22.c for a test that would go wrong.  */
-      if (*pbitsize <= shiftrt
+      if (bs <= shiftrt
 	  || (convert_before_shift
 	      && outer_type && unsignedp != TYPE_UNSIGNED (outer_type)))
 	return NULL_TREE;
-      if (!*preversep ? !BYTES_BIG_ENDIAN : BYTES_BIG_ENDIAN)
-	*pbitpos += shiftrt;
-      *pbitsize -= shiftrt;
+      if (!reversep ? !BYTES_BIG_ENDIAN : BYTES_BIG_ENDIAN)
+	bp += shiftrt;
+      bs -= shiftrt;
     }
 
   /* ... and bit position.  */
   if (!convert_before_shift
-      && outer_type && *pbitsize > TYPE_PRECISION (outer_type))
+      && outer_type && bs > TYPE_PRECISION (outer_type))
     {
-      HOST_WIDE_INT excess = *pbitsize - TYPE_PRECISION (outer_type);
-      if (*preversep ? !BYTES_BIG_ENDIAN : BYTES_BIG_ENDIAN)
-	*pbitpos += excess;
-      *pbitsize -= excess;
+      HOST_WIDE_INT excess = bs - TYPE_PRECISION (outer_type);
+      if (reversep ? !BYTES_BIG_ENDIAN : BYTES_BIG_ENDIAN)
+	bp += excess;
+      bs -= excess;
     }
 
-  *pexp = exp;
-
   /* If the number of bits in the reference is the same as the bitsize of
      the outer type, then the outer type gives the signedness. Otherwise
      (in case of a small bitfield) the signedness is unchanged.  */
-  if (outer_type && *pbitsize == TYPE_PRECISION (outer_type))
-    *punsignedp = TYPE_UNSIGNED (outer_type);
+  if (outer_type && bs == TYPE_PRECISION (outer_type))
+    unsignedp = TYPE_UNSIGNED (outer_type);
 
-  if (pand_mask)
+  /* Make the mask the expected width.  */
+  if (and_mask.get_precision () != 0)
     {
-      /* Make the mask the expected width.  */
-      if (and_mask.get_precision () != 0)
-	{
-	  /* If the AND_MASK encompasses bits that would be extensions of
-	     the sign bit, set *PSIGNBIT.  */
-	  if (!unsignedp
-	      && and_mask.get_precision () > *pbitsize
-	      && (and_mask
-		  & wi::mask (*pbitsize, true, and_mask.get_precision ())) != 0)
-	    *psignbit = true;
-	  and_mask = wide_int::from (and_mask, *pbitsize, UNSIGNED);
-	}
+      /* If the AND_MASK encompasses bits that would be extensions of
+	 the sign bit, set SIGNBIT.  */
+      if (!unsignedp
+	  && and_mask.get_precision () > bs
+	  && (and_mask & wi::mask (bs, true, and_mask.get_precision ())) != 0)
+	signbit = true;
+      and_mask = wide_int::from (and_mask, bs, UNSIGNED);
+    }
 
-      *pand_mask = and_mask;
+  *pexp = exp;
+  *pload = load;
+  *pbitsize = bs;
+  *pbitpos = bp;
+  *punsignedp = unsignedp;
+  *preversep = reversep;
+  *pvolatilep = volatilep;
+  *psignbit = signbit;
+  *pand_mask = and_mask;
+  if (xorp)
+    {
+      *pxorp = xorp;
+      *pxor_cmp_op = xor_cmp_op;
+      *pxor_and_mask = xor_and_mask;
     }
 
   return inner;
@@ -8168,19 +8170,27 @@ fold_truth_andor_for_ifcombine (enum tree_code code, tree truth_type,
 				     &ll_and_mask, &ll_signbit,
 				     &l_xor, &lr_arg, &lr_and_mask,
 				     &ll_load, ll_loc);
+  if (!ll_inner)
+    return 0;
   lr_inner = decode_field_reference (&lr_arg, &lr_bitsize, &lr_bitpos,
 				     &lr_unsignedp, &lr_reversep, &volatilep,
 				     &lr_and_mask, &lr_signbit, &l_xor, 0, 0,
 				     &lr_load, lr_loc);
+  if (!lr_inner)
+    return 0;
   rl_inner = decode_field_reference (&rl_arg, &rl_bitsize, &rl_bitpos,
 				     &rl_unsignedp, &rl_reversep, &volatilep,
 				     &rl_and_mask, &rl_signbit,
 				     &r_xor, &rr_arg, &rr_and_mask,
 				     &rl_load, rl_loc);
+  if (!rl_inner)
+    return 0;
   rr_inner = decode_field_reference (&rr_arg, &rr_bitsize, &rr_bitpos,
 				     &rr_unsignedp, &rr_reversep, &volatilep,
 				     &rr_and_mask, &rr_signbit, &r_xor, 0, 0,
 				     &rr_load, rr_loc);
+  if (!rr_inner)
+    return 0;
 
   /* It must be true that the inner operation on the lhs of each
      comparison must be the same if we are to be able to do anything.
@@ -8188,16 +8198,13 @@ fold_truth_andor_for_ifcombine (enum tree_code code, tree truth_type,
      the rhs's.  If one is a load and the other isn't, we have to be
      conservative and avoid the optimization, otherwise we could get
      SRAed fields wrong.  */
-  if (volatilep
-      || ll_reversep != rl_reversep
-      || ll_inner == 0 || rl_inner == 0)
+  if (volatilep || ll_reversep != rl_reversep)
     return 0;
 
   if (! operand_equal_p (ll_inner, rl_inner, 0))
     {
       /* Try swapping the operands.  */
       if (ll_reversep != rr_reversep
-	  || !rr_inner
 	  || !operand_equal_p (ll_inner, rr_inner, 0))
 	return 0;
 
@@ -8266,7 +8273,6 @@ fold_truth_andor_for_ifcombine (enum tree_code code, tree truth_type,
       lr_reversep = ll_reversep;
     }
   else if (lr_reversep != rr_reversep
-	   || lr_inner == 0 || rr_inner == 0
 	   || ! operand_equal_p (lr_inner, rr_inner, 0)
 	   || ((lr_load && rr_load)
 	       ? gimple_vuse (lr_load) != gimple_vuse (rr_load)
@@ -8520,6 +8526,9 @@ fold_truth_andor_for_ifcombine (enum tree_code code, tree truth_type,
   else
     rl_mask = wi::shifted_mask (xrl_bitpos, rl_bitsize, false, lnprec);
 
+  /* When we set l_const, we also set r_const.  */
+  gcc_checking_assert (!l_const.get_precision () == !r_const.get_precision ());
+
   /* Adjust right-hand constants in both original comparisons to match width
      and bit position.  */
   if (l_const.get_precision ())
@@ -8550,10 +8559,6 @@ fold_truth_andor_for_ifcombine (enum tree_code code, tree truth_type,
 	  return constant_boolean_node (wanted_code == NE_EXPR, truth_type);
 	}
 
-      /* When we set l_const, we also set r_const, so we need not test it
-	 again.  */
-      gcc_checking_assert (r_const.get_precision ());
-
       /* Before clipping upper bits of the right-hand operand of the compare,
 	 check that they're sign or zero extensions, depending on how the
 	 left-hand operand would be extended.  */


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