Mostly rewrite genrecog

[Richard Sandiford]

Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes:
> Hi Richard,
>
> I see regressions with the current IBM z13 vector patchset which appear to be related to the new
> genrecog.
>
> The following two insn definitions only differ in the mode and predicate of the shift count operand.
>
> (define_insn "lshr<mode>3"
>   [(set (match_operand:VI              0 "register_operand"             "=v")
>         (lshiftrt:VI (match_operand:VI 1 "register_operand"              "v")
>                      (match_operand:SI 2 "shift_count_or_setmem_operand" "Y")))]
>   "TARGET_VX"
>   "vesrl<bhfgq>\t%v0,%v1,%Y2"
>   [(set_attr "op_type" "VRS")])
>
> (define_insn "vlshr<mode>3"
>   [(set (match_operand:VI              0 "register_operand" "=v")
>         (lshiftrt:VI (match_operand:VI 1 "register_operand"  "v")
>                      (match_operand:VI 2 "register_operand"  "v")))]
>   "TARGET_VX"
>   "vesrlv<bhfgq>\t%v0,%v1,%v2"
>   [(set_attr "op_type" "VRR")])
>
>
> However, the insn-recog.c code only seem to check the predicate. This is a problem since
> shift_count_or_setmem_operand does not check the mode.

Yeah, it's a bug if a "non-special" predicate doesn't check the mode.
Even old genreog relied on that:

/* After factoring, try to simplify the tests on any one node.
   Tests that are useful for switch statements are recognizable
   by having only a single test on a node -- we'll be manipulating
   nodes with multiple tests:

   If we have mode tests or code tests that are redundant with
   predicates, remove them.  */

although it sounds like the old optimisation didn't trigger for your case.

genpreds.c:mark_mode_tests is supposed to add these tests automatically
if needed.  I suppose it isn't doing so here because the predicate
accepts const_int and because of:

/* 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.  [...]
*/
static void
mark_mode_tests (rtx exp)
{
  switch (GET_CODE (exp))
    {
[...]
    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;
      break;

The code matches the comment, but it doesn't look right.  Perhaps it
was supposed to mean match_codes that _only_ contain const_int and
const_double?  Knowing that the rtx is one of those codes guarantees
that the mode is VOIDmode, but a match_code that includes other rtxes
as well doesn't itself test the mode of those rtxes.

Even then, a predicate that matches const_ints and is passed SImode
mustn't accept const_ints outside the SImode range.  I suppose we
just rely on all predicates to perform some kind of range check.
(The standard ones do of course.)

As a quick workaround, try replacing the case above with:

    case MATCH_CODE:
      if (XSTR (exp, 1)[0] != '\0')
        NO_MODE_TEST (exp) = 1;
      break;

I'll try to come up with a better fix in the meantime.

Thanks,
Richard