[ARM] Optimize compare against smin/umin

Here is an updated patch that addresses the issues you mentioned:

2015-07-24  Michael Collison  <michael.collison@linaro.org
                     Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>

               * gcc/config/arm/arm.md (*arm_smin_cmp): New pattern.
               (*arm_umin_cmp): Likewise.
               * gcc.target/arm/mincmp.c: Test min compare idiom.

Hi Michael,

On 26/07/15 23:54, Michael Collison wrote:
> Here is an updated patch that addresses the issues you mentioned:
>
> 2015-07-24  Michael Collison  <michael.collison@linaro.org
>                       Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>
>                 * gcc/config/arm/arm.md (*arm_smin_cmp): New pattern.
>                 (*arm_umin_cmp): Likewise.
>                 * gcc.target/arm/mincmp.c: Test min compare idiom.

Just "New test." would be a better ChangeLog entry for this.

>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 0be70a8..361c292 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -3455,6 +3455,44 @@
>       (set_attr "type" "multiple,multiple")]
>    )
>
> +;; t = (s/u)min (x, y)
> +;; cc = cmp (t, z)
> +;; is the same as
> +;; cmp x, z
> +;; cmpge(u) y, z
> +
> +(define_insn_and_split "*arm_smin_cmp"
> +  [(set (reg:CC CC_REGNUM)
> +    (compare:CC
> +     (smin:SI (match_operand:SI 0 "s_register_operand" "r")
> +          (match_operand:SI 1 "s_register_operand" "r"))
> +     (match_operand:SI 2 "s_register_operand" "r")))]
> +  "TARGET_32BIT"
> +  "#"
> +  "&& reload_completed"
> +  [(set (reg:CC CC_REGNUM)
> +    (compare:CC (match_dup 0) (match_dup 2)))
> +   (cond_exec (ge:CC (reg:CC CC_REGNUM) (const_int 0))
> +          (set (reg:CC CC_REGNUM)
> +           (compare:CC (match_dup 1) (match_dup 2))))]
> +)
> +
> +(define_insn_and_split "*arm_umin_cmp"
> +  [(set (reg:CC CC_REGNUM)
> +    (compare:CC
> +     (umin:SI (match_operand:SI 0 "s_register_operand" "r")
> +          (match_operand:SI 1 "s_register_operand" "r"))
> +     (match_operand:SI 2 "s_register_operand" "r")))]
> +  "TARGET_32BIT"
> +  "#"
> +  "&& reload_completed"
> +  [(set (reg:CC CC_REGNUM)
> +    (compare:CC (match_dup 0) (match_dup 2)))
> +   (cond_exec (geu:CC (reg:CC CC_REGNUM) (const_int 0))
> +          (set (reg:CC CC_REGNUM)
> +           (compare:CC (match_dup 1) (match_dup 2))))]
> +)
> +
>    (define_expand "umaxsi3"
>      [(parallel [
>        (set (match_operand:SI 0 "s_register_operand" "")
> diff --git a/gcc/testsuite/gcc.target/arm/mincmp.c
> b/gcc/testsuite/gcc.target/arm/mincmp.c
> new file mode 100644
> index 0000000..2a55c6d
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/arm/mincmp.c
> @@ -0,0 +1,18 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +/* { dg-require-effective-target arm32 } */
> +
> +#define min(x, y) ((x) <= (y)) ? (x) : (y)
> +
> +unsigned int foo (unsigned int i, unsigned int x ,unsigned int y)

Comma after "unsigned int x" in the wrong place.

> +{
> +  return i < (min (x, y));
> +}
> +
> +int bar (int i, int x, int y)
> +{
> +  return i < (min (x, y));
> +}

Can you please use GNU style for the testcase.
New line after return type, function name an arguments on their own line.

Ok with these changes.

Thanks,
Kyrill

Hi Michael,

It looks like this patch introduces regressions on armeb in:
gcc.dg/vect/vect-reduc-7.c
gcc.dg/vect/vect-reduc-8.c
gcc.dg/vect/vect-reduc-9.c

See http://people.linaro.org/~christophe.lyon/cross-validation/gcc/trunk/226476/report-build-info.html
for a bit more details.

Can you have a look?

Thanks,

Christophe.


On 27 July 2015 at 10:17, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:
> Hi Michael,
>
> On 26/07/15 23:54, Michael Collison wrote:
>>
>> Here is an updated patch that addresses the issues you mentioned:
>>
>> 2015-07-24  Michael Collison  <michael.collison@linaro.org
>>                       Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>>
>>                 * gcc/config/arm/arm.md (*arm_smin_cmp): New pattern.
>>                 (*arm_umin_cmp): Likewise.
>>                 * gcc.target/arm/mincmp.c: Test min compare idiom.
>
>
> Just "New test." would be a better ChangeLog entry for this.
>
>
>>
>> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
>> index 0be70a8..361c292 100644
>> --- a/gcc/config/arm/arm.md
>> +++ b/gcc/config/arm/arm.md
>> @@ -3455,6 +3455,44 @@
>>       (set_attr "type" "multiple,multiple")]
>>    )
>>
>> +;; t = (s/u)min (x, y)
>> +;; cc = cmp (t, z)
>> +;; is the same as
>> +;; cmp x, z
>> +;; cmpge(u) y, z
>> +
>> +(define_insn_and_split "*arm_smin_cmp"
>> +  [(set (reg:CC CC_REGNUM)
>> +    (compare:CC
>> +     (smin:SI (match_operand:SI 0 "s_register_operand" "r")
>> +          (match_operand:SI 1 "s_register_operand" "r"))
>> +     (match_operand:SI 2 "s_register_operand" "r")))]
>> +  "TARGET_32BIT"
>> +  "#"
>> +  "&& reload_completed"
>> +  [(set (reg:CC CC_REGNUM)
>> +    (compare:CC (match_dup 0) (match_dup 2)))
>> +   (cond_exec (ge:CC (reg:CC CC_REGNUM) (const_int 0))
>> +          (set (reg:CC CC_REGNUM)
>> +           (compare:CC (match_dup 1) (match_dup 2))))]
>> +)
>> +
>> +(define_insn_and_split "*arm_umin_cmp"
>> +  [(set (reg:CC CC_REGNUM)
>> +    (compare:CC
>> +     (umin:SI (match_operand:SI 0 "s_register_operand" "r")
>> +          (match_operand:SI 1 "s_register_operand" "r"))
>> +     (match_operand:SI 2 "s_register_operand" "r")))]
>> +  "TARGET_32BIT"
>> +  "#"
>> +  "&& reload_completed"
>> +  [(set (reg:CC CC_REGNUM)
>> +    (compare:CC (match_dup 0) (match_dup 2)))
>> +   (cond_exec (geu:CC (reg:CC CC_REGNUM) (const_int 0))
>> +          (set (reg:CC CC_REGNUM)
>> +           (compare:CC (match_dup 1) (match_dup 2))))]
>> +)
>> +
>>    (define_expand "umaxsi3"
>>      [(parallel [
>>        (set (match_operand:SI 0 "s_register_operand" "")
>> diff --git a/gcc/testsuite/gcc.target/arm/mincmp.c
>> b/gcc/testsuite/gcc.target/arm/mincmp.c
>> new file mode 100644
>> index 0000000..2a55c6d
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/arm/mincmp.c
>> @@ -0,0 +1,18 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2" } */
>> +/* { dg-require-effective-target arm32 } */
>> +
>> +#define min(x, y) ((x) <= (y)) ? (x) : (y)
>> +
>> +unsigned int foo (unsigned int i, unsigned int x ,unsigned int y)
>
>
> Comma after "unsigned int x" in the wrong place.
>
>> +{
>> +  return i < (min (x, y));
>> +}
>> +
>> +int bar (int i, int x, int y)
>> +{
>> +  return i < (min (x, y));
>> +}
>
>
> Can you please use GNU style for the testcase.
> New line after return type, function name an arguments on their own line.
>
> Ok with these changes.
>
> Thanks,
> Kyrill
>