Re: Mostly rewrite genrecog

[Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>]

On 05/17/2015 11:12 PM, Richard Sandiford wrote:
> 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 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-