Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)

[Andrew Pinski <pinskia at gmail dot com>]

On Sun, Nov 23, 2014 at 9:11 PM, Zhenqiang Chen <zhenqiang.chen@arm.com> wrote:
> Hi,
>
> Expand pass always uses sign-extend to represent constant value. For the
> case in the patch, a 8-bit unsigned value "252" is represented as "-4",
> which pass the ccmn check. After mode conversion, "-4" becomes "252", which
> leads to mismatch.
>
> The patch adds another operand check after mode conversion.
>
> No make check regression with qemu.
>
> OK for trunk?
>
> Thanks!
> -Zhenqiang
>
> ChangeLog:
> 2014-11-24  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>
>         PR target/64015
>         * config/aarch64/aarch64.c (aarch64_gen_ccmp_first): Recheck operand
>         after mode conversion.
>         (aarch64_gen_ccmp_next): Likewise.
>
> testsuite/ChangeLog:
> 2014-11-24  Zhenqiang Chen  <zhenqiang.chen@arm.com>
>
>         * gcc.target/aarch64/pr64015.c: New test.
>
> diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
> index 1809513..203d095 100644
> --- a/gcc/config/aarch64/aarch64.c
> +++ b/gcc/config/aarch64/aarch64.c
> @@ -10311,7 +10311,10 @@ aarch64_gen_ccmp_first (int code, rtx op0, rtx op1)
>    if (!aarch64_plus_operand (op1, GET_MODE (op1)))
>      op1 = force_reg (mode, op1);
>
> -  if (!aarch64_convert_mode (&op0, &op1, unsignedp))
> +  if (!aarch64_convert_mode (&op0, &op1, unsignedp)
> +        /* Some negative value might be transformed into a positive one.
> +           So need recheck here.  */
> +      || !aarch64_plus_operand (op1, GET_MODE (op1)))
>      return NULL_RTX;

Shouldn't we force it to a reg here instead?

>
>    mode = aarch64_code_to_ccmode ((enum rtx_code) code);
> @@ -10344,7 +10347,10 @@ aarch64_gen_ccmp_next (rtx prev, int cmp_code, rtx
> op0, rtx op1, int bit_code)
>        || !aarch64_ccmp_operand (op1, GET_MODE (op1)))
>      return NULL_RTX;
>
> -  if (!aarch64_convert_mode (&op0, &op1, unsignedp))
> +  if (!aarch64_convert_mode (&op0, &op1, unsignedp)
> +        /* Some negative value might be transformed into a positive one.
> +           So need recheck here.  */
> +      || !aarch64_ccmp_operand (op1, GET_MODE (op1)))
>      return NULL_RTX;


Also really the cost of forcing to a register is better really.
In the cases where we would not have forced to a register in a cmp
instruction, the constant would be one instruction and compared to the
cost of two cset and an and/or is better.
In the cases where we would have forced to a register for the cmp
instruction, two cost for doing the forcing is the same on both cases
but since we gaining from removing a cset and an and/or we are better.

>
>    mode = aarch64_code_to_ccmode ((enum rtx_code) cmp_code);
>
> diff --git a/gcc/testsuite/gcc.target/aarch64/pr64015.c
> b/gcc/testsuite/gcc.target/aarch64/pr64015.c
> new file mode 100644
> index 0000000..eeed665
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/pr64015.c
> @@ -0,0 +1,7 @@
> +/* { dg-do compile } */
> +/* { dg-options " -O2 " } */
> +int
> +test (unsigned short a, unsigned char b)
> +{
> +  return a > 0xfff2 && b > 252;
> +}

Since this testcase is generic (except for the -O2), it really should
go into gcc.c-torture/compile instead of remove the two dg-*
directives so it can be tested on more than AARCH64 and on more
optimization levels.


Thanks,
Andrew Pinski

>
>
>