Re: [ARM] Optimize compare against smin/umin

[Ramana Radhakrishnan <ramana.gcc@googlemail.com>]

On Thu, Jun 25, 2015 at 6:08 PM, Michael Collison
<michael.collison@linaro.org> wrote:
>
> This patch is designed to optimize constructs such as:
>
> #define min(x, y) ((x) <= (y)) ? (x) : (y)
>
> unsignedint  foo (unsignedint  i, unsignedint  x ,unsignedint  y)
> {
>   return  i < (min (x, y));
> }
>
> int  bar (int  i,int  x,int  y)
> {
>   return  i < (min (x, y));
> }
>
> Patch was tested on arm-linux-gnueabi, arm-linux-gnueabihf,
> armeb-linux-gnueabihf. Okay for trunk?

Sorry about the slow review and I wanted someone else to look at it
given I had a hand in writing this patch up.

Please add a testcase.

>
>
> 2015-06-24  Michael Collison  <michael.collison@linaro.org
>
>     2012-05-01  Ramana Radhakrishnan <ramana.radhakrishnan@linaro.org>

Please fix the Changelog formatting here.

>
>     * gcc/config/arm/arm.md (*arm_smin_cmp): New pattern.
>     (*arm_umin_cmp): Likewise.
>
> diff --git a/gcc/config/arm/arm.md b/gcc/config/arm/arm.md
> index 1ac8af0..994c95f 100644
> --- a/gcc/config/arm/arm.md
> +++ b/gcc/config/arm/arm.md
> @@ -3455,6 +3455,28 @@
>     (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"
> +  "#"
> +  ""
> +  [(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))))]
> +)


IIUC it's not entirely safe to have cond_execs in the instruction
stream prior to reload - I think the consensus was that spilling and
filling with cond-exec style instructions could end up with
non-cond-exec style spills thus destroying registers in the non
cond-exec cases. so, lets just add a reload_completed to be safe here.

See https://patches.linaro.org/6469/ for more on this topic.

> +
>  (define_expand "umaxsi3"
>    [(parallel [
>      (set (match_operand:SI 0 "s_register_operand" "")
> @@ -3521,6 +3543,22 @@
>     (set_attr "type" "store1")]
>  )
>
> +(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"
> +  "#"
> +  ""
> +  [(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))))]
> +)
> +

Please move this below the other pattern.

>  (define_insn "*store_minmaxsi"
>    [(set (match_operand:SI 0 "memory_operand" "=m")
>      (match_operator:SI 3 "minmax_operator"
>
> --
> Michael Collison
> Linaro Toolchain Working Group
> michael.collison@linaro.org
>


Please repost after testing those changes and then I think this is OK to go in.

regards
Ramana