Andreas Krebbel <kreb...@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
