Andreas Krebbel <kreb...@linux.vnet.ibm.com> writes:
> 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.
OK. After a false start earlier in the week, I've now got a patch
that I think fixes genpreds.c "properly". Hope to post it soon.
Thanks,
Richard
