Re: [ARM] Optimize compare against smin/umin

[Christophe Lyon <christophe.lyon@linaro.org>]

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
>