On 05/17/2015 11:12 PM, Richard Sandiford wrote:
> 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 for looking into this!
I've applied a patch adding a mode check to shift_count_or_setmem_operand which
as expected fixes
the issue for me.
-Andreas-
