diff mbox

RFA: Fix mode checks for possibly-constant predicates

Message ID 87pp5jl2m1.fsf_-_@e105548-lin.cambridge.arm.com
State New
Headers show

Commit Message

Richard Sandiford May 29, 2015, 5:23 p.m. UTC 
genrecog relies on a predicate foo_operand (op, mode) checking that OP
really does have mode MODE, with VOIDmode acting as a wildcard.  This was
true even with the old genrecog, but as Andreas found on s390x, new genrecog
is being a bit more aggressive about it.

The problem is that at the moment, genpreds drops the mode check for
anything that can accept a CONST_INT or CONST_DOUBLE, on the basis that
constant integers have a recorded mode of VOIDmode whatever their "real"
logical mode is.  But that means that a predicate that accepts constant
integers and other rtxes like REG won't check the modes of those other
rtxes either.  Even dropping the check for CONST_DOUBLE is wrong, since it
can be a floating-point constant with a real mode.

This patch therefore adjusts the automatic mode test to allow
GET_MODE (op) == VOIDmode in cases where that's necessary.
It also includes CONST_WIDE_INT in the list of constant integer codes.

Bootstrapped & regression-tested on x86_64-linux-gnu.  OK to install?

Thanks,
Richard


gcc/
	* genpreds.c (mark_mode_tests): Mark all MATCH_CODEs as
	NO_MODE_TEST.
	(add_mode_tests): Don't add mode tests if the predicate only
	accepts scalar constant integers.  Otherwise, allow the mode
	of "op" to be VOIDmode if the predicate does accept such integers.

Comments

Richard Henderson May 29, 2015, 5:52 p.m. UTC | #1 
On 05/29/2015 10:23 AM, Richard Sandiford wrote:
> +  /* Check whether the predicate accepts const scalar ints (which always
> +     have a stored mode of VOIDmode, but logically have a real mode)
> +     and whether it matches anything besides const scalar ints.  */
> +  bool matches_const_scalar_int_p = false;
> +  bool matches_other_p = false;
> +  for (int i = 0; i < NUM_RTX_CODE; ++i)
> +    if (p->codes[i])
> +      switch (i)
> +	{
> +	CASE_CONST_SCALAR_INT:
> +	  matches_const_scalar_int_p = true;
> +	  break;
> +
> +	default:
> +	  matches_other_p = true;
> +	  break;
> +	}
> +
> +  /* There's no need for a mode check if the predicate only accepts
> +     constant integers.  The code checks in the predicate are enough
> +     to establish that the mode is VOIDmode.
> +
> +     Note that the predicate itself should check whether a scalar
> +     integer is in range of the given mode.  */
> +  if (!matches_other_p && !p->codes[CONST_DOUBLE])
> +    return;

I think perhaps it would be cleaner to not use CASE_CONST_SCALAR_INT,
and then do

  switch (i)
    {
    case CONST_INT:
    case CONST_WIDE_INT:
      matches_const_scalar_int_p = true;
      break;

    case CONST_DOUBLE:
      if (!TARGET_SUPPORTS_WIDE_INT)
        matches_const_scalar_int_p = true;
      matches_other_p = true;
      break;

    default:
      matches_other_p = true;
      break;
    }

  if (!matches_other_p)
    return;

Otherwise ok.


r~
diff mbox

Patch

Index: gcc/genpreds.c
===================================================================
--- gcc/genpreds.c	2015-05-22 16:49:55.454319576 +0100
+++ gcc/genpreds.c	2015-05-23 10:46:58.187370110 +0100
@@ -218,11 +218,11 @@  needs_variable (rtx exp, const char *var
 
 /* Given an RTL expression EXP, find all subexpressions which we may
    assume to perform mode tests.  Normal MATCH_OPERAND does;
-   MATCH_CODE does if it applies to the whole expression and accepts
-   CONST_INT or CONST_DOUBLE; and we have to assume that MATCH_TEST
-   does not.  These combine in almost-boolean fashion - the only
-   exception is that (not X) must be assumed not to perform a mode
-   test, whether or not X does.
+   MATCH_CODE doesn't as such (although certain codes always have
+   VOIDmode); and we have to assume that MATCH_TEST does not.
+   These combine in almost-boolean fashion - the only exception is
+   that (not X) must be assumed not to perform a mode test, whether
+   or not X does.
 
    The mark is the RTL /v flag, which is true for subexpressions which
    do *not* perform mode tests.
@@ -244,10 +244,7 @@  mark_mode_tests (rtx exp)
       break;
 
     case MATCH_CODE:
-      if (XSTR (exp, 1)[0] != '\0'
-	  || (!strstr (XSTR (exp, 0), "const_int")
-	      && !strstr (XSTR (exp, 0), "const_double")))
-	NO_MODE_TEST (exp) = 1;
+      NO_MODE_TEST (exp) = 1;
       break;
 
     case MATCH_TEST:
@@ -313,6 +310,33 @@  add_mode_tests (struct pred_data *p)
   if (p->special)
     return;
 
+  /* Check whether the predicate accepts const scalar ints (which always
+     have a stored mode of VOIDmode, but logically have a real mode)
+     and whether it matches anything besides const scalar ints.  */
+  bool matches_const_scalar_int_p = false;
+  bool matches_other_p = false;
+  for (int i = 0; i < NUM_RTX_CODE; ++i)
+    if (p->codes[i])
+      switch (i)
+	{
+	CASE_CONST_SCALAR_INT:
+	  matches_const_scalar_int_p = true;
+	  break;
+
+	default:
+	  matches_other_p = true;
+	  break;
+	}
+
+  /* There's no need for a mode check if the predicate only accepts
+     constant integers.  The code checks in the predicate are enough
+     to establish that the mode is VOIDmode.
+
+     Note that the predicate itself should check whether a scalar
+     integer is in range of the given mode.  */
+  if (!matches_other_p && !p->codes[CONST_DOUBLE])
+    return;
+
   mark_mode_tests (p->exp);
 
   /* If the whole expression already tests the mode, we're done.  */
@@ -320,7 +344,11 @@  add_mode_tests (struct pred_data *p)
     return;
 
   match_test_exp = rtx_alloc (MATCH_TEST);
-  XSTR (match_test_exp, 0) = "mode == VOIDmode || GET_MODE (op) == mode";
+  if (matches_const_scalar_int_p)
+    XSTR (match_test_exp, 0) = ("mode == VOIDmode || GET_MODE (op) == mode"
+				" || GET_MODE (op) == VOIDmode");
+  else
+    XSTR (match_test_exp, 0) = "mode == VOIDmode || GET_MODE (op) == mode";
   and_exp = rtx_alloc (AND);
   XEXP (and_exp, 1) = match_test_exp;