[AArch64] Use CC_Z and CC_NZ with csinc and similar instructions

On 18/08/14 19:52, Richard Henderson wrote:
> On 08/18/2014 05:24 AM, Kyrill Tkachov wrote:
>> -(define_insn "*csinc2<mode>_insn"
>> +(define_insn "*csinc2<mode>_<CC_ZERO:mode>_insn"
>>     [(set (match_operand:GPI 0 "register_operand" "=r")
>>           (plus:GPI (match_operator:GPI 2 "aarch64_comparison_operator"
>> -		  [(match_operand:CC 3 "cc_register" "") (const_int 0)])
>> +		  [(match_operand:CC_ZERO 3 "cc_register" "") (const_int 0)])
> Rather than create 3 patterns that only vary in this mode, it would be better
> to use (match_operand 3 "cc_register_zero").  Where the predicate is defined
> with define_special_predicate so that you move the mode check inside the predicate.
>

Like this? I define in a similar way to cc_register, but add an extra 
ior for the CC_Z and CC_NZmode.

Kyrill

2014-08-19  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * config/aarch64/predicates.md (cc_register_zero): New special
     predicate.
     * config/aarch64/aarch64.md (*csinc2<mode>_insn): Use cc_register_zero
     predicate instead of cc_register.
     (csinc3<mode>_insn): Likewise.
     (*csinv3<mode>_insn): Likewise.
     (*csneg3<mode>_insn): Likewise.

> r~
>

On 08/19/2014 06:29 AM, Kyrill Tkachov wrote:
> +(define_special_predicate "cc_register_zero"
> +  (and (match_code "reg")
> +       (and (match_test "REGNO (op) == CC_REGNUM")
> +	    (ior (match_test "mode == GET_MODE (op)")
> +		 (ior (match_test "mode == VOIDmode
> +		                   && GET_MODE_CLASS (GET_MODE (op)) == MODE_CC")
> +		      (match_test "mode == CCmode || mode == CC_Zmode
> +		                   || mode == CC_NZmode")))))
> +)

Well, you're being too liberal with the modes you're accepting, since 'mode'
will always be VOIDmode.

And personally I find those match_tests quite ugly.  Better as:

(define_special_predicate "cc_register_zero"
  (match_code "reg")
{
  return (REGNO (op) == CC_REGNUM
          && (GET_MODE (op) == CCmode
              || GET_MODE (op) == CC_Zmode
              || GET_MODE (op) == CC_NZmode));
})


r~

> (define_special_predicate "cc_register_zero"
>   (match_code "reg")
> {
>   return (REGNO (op) == CC_REGNUM
>           && (GET_MODE (op) == CCmode
>               || GET_MODE (op) == CC_Zmode
>               || GET_MODE (op) == CC_NZmode));
> })

... and now that I read the backend more closely, I see "_zero" was a bad name.

But more importantly, I see no connection between the comparison used and the
CCmode being accepted.  And if we fix that, why are you restricting to just Z
and NZ?  What's wrong with e.g. CFPmode?

In the i386 backend, we check comparison+mode correspondence like

  (match_operator 4 "ix86_carry_flag_operator"
     [(match_operand 3 "flags_reg_operand") (const_int 0)])

I think you'll want something similar.  In the case of CSINC, we can accept all
conditions, so let's start with the most general:

  (match_operator:GPI 2 "aarch64_comparison_operation"
    [(reg CC_REGNUM) (const_int 0)]

or even

  (match_operand:GPI 2 "aarch64_comparison_operation" "")

with

(define_predicate "aarch64_comparison_operation"
    (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,"
                "unordered,ordered,unlt,unle,unge,ungt")
{
  if (XEXP (op, 1) != const0_rtx)
    return false;
  rtx op0 = XEXP (op, 0);
  if (!REG_P (op0) || REGNO (op0) != CC_REGNUM)
    return false;
  return aarch64_get_condition_code (op) >= 0;
})

where aarch64_get_condition_code is
  (1) exported
  (2) adjusted to return "int" not "unsigned"
  (3) adjusted to not abort, but return -1 for invalid combinations.

and the two existing users of aarch64_get_condition_code are adjusted to
gcc_assert that the return value is valid.


r~

On 19/08/14 17:09, Richard Henderson wrote:
>> (define_special_predicate "cc_register_zero"
>>    (match_code "reg")
>> {
>>    return (REGNO (op) == CC_REGNUM
>>            && (GET_MODE (op) == CCmode
>>                || GET_MODE (op) == CC_Zmode
>>                || GET_MODE (op) == CC_NZmode));
>> })
> ... and now that I read the backend more closely, I see "_zero" was a bad name.
>
> But more importantly, I see no connection between the comparison used and the
> CCmode being accepted.  And if we fix that, why are you restricting to just Z
> and NZ?  What's wrong with e.g. CFPmode?
>
> In the i386 backend, we check comparison+mode correspondence like
>
>    (match_operator 4 "ix86_carry_flag_operator"
>       [(match_operand 3 "flags_reg_operand") (const_int 0)])
>
> I think you'll want something similar.  In the case of CSINC, we can accept all
> conditions, so let's start with the most general:
>
>    (match_operator:GPI 2 "aarch64_comparison_operation"
>      [(reg CC_REGNUM) (const_int 0)]
>
> or even
>
>    (match_operand:GPI 2 "aarch64_comparison_operation" "")
>
> with
>
> (define_predicate "aarch64_comparison_operation"
>      (match_code "eq,ne,le,lt,ge,gt,geu,gtu,leu,ltu,"
>                  "unordered,ordered,unlt,unle,unge,ungt")
> {
>    if (XEXP (op, 1) != const0_rtx)
>      return false;
>    rtx op0 = XEXP (op, 0);
>    if (!REG_P (op0) || REGNO (op0) != CC_REGNUM)
>      return false;
>    return aarch64_get_condition_code (op) >= 0;
> })
>
> where aarch64_get_condition_code is
>    (1) exported
>    (2) adjusted to return "int" not "unsigned"
>    (3) adjusted to not abort, but return -1 for invalid combinations.
>
> and the two existing users of aarch64_get_condition_code are adjusted to
> gcc_assert that the return value is valid.

Hmm, when I do that combine refuses to match the csinc patterns:
Failed to match this instruction:
(set (reg:SI 73 [ D.2564 ])
     (if_then_else:SI (ne (reg:CC 66 cc)
             (const_int 0 [0]))
         (plus:SI (reg:SI 74 [ D.2565 ])
             (const_int 1 [0x1]))
         (const_int 0 [0])))

I probably mistyped something, I'll look into it...
Thanks for the help,
Kyrill


>
>
> r~
>