Re: [PATCH PR81647][AARCH64] Fix handling of Unordered Comparisons in aarch64-simd.md

[Christophe Lyon <christophe dot lyon at linaro dot org>]

On 19 March 2018 at 19:55, Sudakshina Das <sudi.das@arm.com> wrote:
> Hi
>
>
> On 19/03/18 14:29, James Greenhalgh wrote:
>>
>> On Fri, Dec 15, 2017 at 11:57:46AM +0000, Sudi Das wrote:
>>>
>>> Hi
>>>
>>> This patch fixes the inconsistent behavior observed at -O3 for the
>>> unordered comparisons. According to the online docs
>>>
>>> (https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gccint/Unary-and-Binary-Expressions.html),
>>> all of the following should not raise an FP exception:
>>> - UNGE_EXPR
>>> - UNGT_EXPR
>>> - UNLE_EXPR
>>> - UNLT_EXPR
>>> - UNEQ_EXPR
>>> Also ORDERED_EXPR and UNORDERED_EXPR should only return zero or one.
>>>
>>> The aarch64-simd.md handling of these were generating exception raising
>>> instructions such as fcmgt. This patch changes the instructions that are
>>> emitted to in order to not give out the exceptions. We first check each
>>> operand for NaNs and force any elements containing NaN to zero before
>>> using them in the compare.
>>>
>>> Example: UN<cc> (a, b) -> UNORDERED (a, b) | (cm<cc> (isnan (a) ? 0.0 :
>>> a, isnan (b) ? 0.0 : b))
>>>
>>>
>>> The ORDERED_EXPR is now handled as (cmeq (a, a) & cmeq (b, b)) and
>>> UNORDERED_EXPR as ~ORDERED_EXPR and UNEQ as (~ORDERED_EXPR | cmeq (a,b)).
>>>
>>> Testing done: Checked for regressions on bootstrapped
>>> aarch64-none-linux-gnu and added a new test case.
>>>
>>> Is this ok for trunk? This will probably need a back-port to
>>> gcc-7-branch as well.
>>
>>
>> OK.
>>
>> Let it soak on trunk for a while before the backport.
>
>
> Thanks. Committed to trunk as r258653. Will wait a week before backport.
>

Hi,

As the test failed to compile on aarch64 bare-metal targets, I added
/* { dg-require-effective-target fenv_exceptions } */
as obvious (r258672).

2018-03-20  Christophe Lyon  <christophe.lyon@linaro.org>

       PR target/81647
       * gcc.target/aarch64/pr81647.c: Require fenv_exceptions.

Index: testsuite/gcc.target/aarch64/pr81647.c
===================================================================
--- testsuite/gcc.target/aarch64/pr81647.c      (revision 258671)
+++ testsuite/gcc.target/aarch64/pr81647.c      (revision 258672)
@@ -1,5 +1,6 @@
 /* { dg-do run } */
 /* { dg-options "-O3 -fdump-tree-ssa" } */
+/* { dg-require-effective-target fenv_exceptions } */

 #include <fenv.h>



Christophe

> Sudi
>
>
>>
>> Thanks,
>> James
>>
>>> ChangeLog Entries:
>>>
>>> *** gcc/ChangeLog ***
>>>
>>> 2017-12-15  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>         PR target/81647
>>>         * config/aarch64/aarch64-simd.md (vec_cmp<mode><v_int_equiv>):
>>> Modify
>>> instructions for
>>>         UNLT, UNLE, UNGT, UNGE, UNEQ, UNORDERED and ORDERED.
>>>
>>> *** gcc/testsuite/ChangeLog ***
>>>
>>> 2017-12-15  Sudakshina Das  <sudi.das@arm.com>
>>>
>>>         PR target/81647
>>>         * gcc.target/aarch64/pr81647.c: New.
>>
>>
>>> diff --git a/gcc/config/aarch64/aarch64-simd.md
>>> b/gcc/config/aarch64/aarch64-simd.md
>>> index
>>> f90f74fe7fd5990a97b9f4eb68f5735b7d4fb9aa..acff06c753b3e3aaa5775632929909afa4d3294b
>>> 100644
>>> --- a/gcc/config/aarch64/aarch64-simd.md
>>> +++ b/gcc/config/aarch64/aarch64-simd.md
>>> @@ -2731,10 +2731,10 @@
>>>           break;
>>>         }
>>>         /* Fall through.  */
>>> -    case UNGE:
>>> +    case UNLT:
>>>         std::swap (operands[2], operands[3]);
>>>         /* Fall through.  */
>>> -    case UNLE:
>>> +    case UNGT:
>>>       case GT:
>>>         comparison = gen_aarch64_cmgt<mode>;
>>>         break;
>>> @@ -2745,10 +2745,10 @@
>>>           break;
>>>         }
>>>         /* Fall through.  */
>>> -    case UNGT:
>>> +    case UNLE:
>>>         std::swap (operands[2], operands[3]);
>>>         /* Fall through.  */
>>> -    case UNLT:
>>> +    case UNGE:
>>>       case GE:
>>>         comparison = gen_aarch64_cmge<mode>;
>>>         break;
>>> @@ -2771,21 +2771,35 @@
>>>       case UNGT:
>>>       case UNLE:
>>>       case UNLT:
>>> -    case NE:
>>> -      /* FCM returns false for lanes which are unordered, so if we use
>>> -        the inverse of the comparison we actually want to emit, then
>>> -        invert the result, we will end up with the correct result.
>>> -        Note that a NE NaN and NaN NE b are true for all a, b.
>>> -
>>> -        Our transformations are:
>>> -        a UNGE b -> !(b GT a)
>>> -        a UNGT b -> !(b GE a)
>>> -        a UNLE b -> !(a GT b)
>>> -        a UNLT b -> !(a GE b)
>>> -        a   NE b -> !(a EQ b)  */
>>> -      gcc_assert (comparison != NULL);
>>> -      emit_insn (comparison (operands[0], operands[2], operands[3]));
>>> -      emit_insn (gen_one_cmpl<v_int_equiv>2 (operands[0], operands[0]));
>>> +      {
>>> +       /* All of the above must not raise any FP exceptions.  Thus we
>>> first
>>> +          check each operand for NaNs and force any elements containing
>>> NaN to
>>> +          zero before using them in the compare.
>>> +          Example: UN<cc> (a, b) -> UNORDERED (a, b) |
>>> +                                    (cm<cc> (isnan (a) ? 0.0 : a,
>>> +                                             isnan (b) ? 0.0 : b))
>>> +          We use the following transformations for doing the
>>> comparisions:
>>> +          a UNGE b -> a GE b
>>> +          a UNGT b -> a GT b
>>> +          a UNLE b -> b GE a
>>> +          a UNLT b -> b GT a.  */
>>> +
>>> +       rtx tmp0 = gen_reg_rtx (<V_INT_EQUIV>mode);
>>> +       rtx tmp1 = gen_reg_rtx (<V_INT_EQUIV>mode);
>>> +       rtx tmp2 = gen_reg_rtx (<V_INT_EQUIV>mode);
>>> +       emit_insn (gen_aarch64_cmeq<mode> (tmp0, operands[2],
>>> operands[2]));
>>> +       emit_insn (gen_aarch64_cmeq<mode> (tmp1, operands[3],
>>> operands[3]));
>>> +       emit_insn (gen_and<v_int_equiv>3 (tmp2, tmp0, tmp1));
>>> +       emit_insn (gen_and<v_int_equiv>3 (tmp0, tmp0,
>>> +                       lowpart_subreg (<V_INT_EQUIV>mode, operands[2],
>>> <MODE>mode)));
>>> +       emit_insn (gen_and<v_int_equiv>3 (tmp1, tmp1,
>>> +                       lowpart_subreg (<V_INT_EQUIV>mode, operands[3],
>>> <MODE>mode)));
>>> +       gcc_assert (comparison != NULL);
>>> +       emit_insn (comparison (operands[0],
>>> +                              lowpart_subreg (<MODE>mode, tmp0,
>>> <V_INT_EQUIV>mode),
>>> +                              lowpart_subreg (<MODE>mode, tmp1,
>>> <V_INT_EQUIV>mode)));
>>> +       emit_insn (gen_orn<v_int_equiv>3 (operands[0], tmp2,
>>> operands[0]));
>>> +      }
>>>         break;
>>>         case LT:
>>> @@ -2793,25 +2807,19 @@
>>>       case GT:
>>>       case GE:
>>>       case EQ:
>>> +    case NE:
>>>         /* The easy case.  Here we emit one of FCMGE, FCMGT or FCMEQ.
>>>          As a LT b <=> b GE a && a LE b <=> b GT a.  Our transformations
>>> are:
>>>          a GE b -> a GE b
>>>          a GT b -> a GT b
>>>          a LE b -> b GE a
>>>          a LT b -> b GT a
>>> -        a EQ b -> a EQ b  */
>>> +        a EQ b -> a EQ b
>>> +        a NE b -> ~(a EQ b)  */
>>>         gcc_assert (comparison != NULL);
>>>         emit_insn (comparison (operands[0], operands[2], operands[3]));
>>> -      break;
>>> -
>>> -    case UNEQ:
>>> -      /* We first check (a > b ||  b > a) which is !UNEQ, inverting
>>> -        this result will then give us (a == b || a UNORDERED b).  */
>>> -      emit_insn (gen_aarch64_cmgt<mode> (operands[0],
>>> -                                        operands[2], operands[3]));
>>> -      emit_insn (gen_aarch64_cmgt<mode> (tmp, operands[3],
>>> operands[2]));
>>> -      emit_insn (gen_ior<v_int_equiv>3 (operands[0], operands[0], tmp));
>>> -      emit_insn (gen_one_cmpl<v_int_equiv>2 (operands[0], operands[0]));
>>> +      if (code == NE)
>>> +       emit_insn (gen_one_cmpl<v_int_equiv>2 (operands[0],
>>> operands[0]));
>>>         break;
>>>         case LTGT:
>>> @@ -2823,21 +2831,22 @@
>>>         emit_insn (gen_ior<v_int_equiv>3 (operands[0], operands[0],
>>> tmp));
>>>         break;
>>>   -    case UNORDERED:
>>> -      /* Operands are ORDERED iff (a > b || b >= a), so we can compute
>>> -        UNORDERED as !ORDERED.  */
>>> -      emit_insn (gen_aarch64_cmgt<mode> (tmp, operands[2],
>>> operands[3]));
>>> -      emit_insn (gen_aarch64_cmge<mode> (operands[0],
>>> -                                        operands[3], operands[2]));
>>> -      emit_insn (gen_ior<v_int_equiv>3 (operands[0], operands[0], tmp));
>>> -      emit_insn (gen_one_cmpl<v_int_equiv>2 (operands[0], operands[0]));
>>> -      break;
>>> -
>>>       case ORDERED:
>>> -      emit_insn (gen_aarch64_cmgt<mode> (tmp, operands[2],
>>> operands[3]));
>>> -      emit_insn (gen_aarch64_cmge<mode> (operands[0],
>>> -                                        operands[3], operands[2]));
>>> -      emit_insn (gen_ior<v_int_equiv>3 (operands[0], operands[0], tmp));
>>> +    case UNORDERED:
>>> +    case UNEQ:
>>> +      /* cmeq (a, a) & cmeq (b, b).  */
>>> +      emit_insn (gen_aarch64_cmeq<mode> (operands[0],
>>> +                                        operands[2], operands[2]));
>>> +      emit_insn (gen_aarch64_cmeq<mode> (tmp, operands[3],
>>> operands[3]));
>>> +      emit_insn (gen_and<v_int_equiv>3 (operands[0], operands[0], tmp));
>>> +
>>> +      if (code == UNORDERED)
>>> +       emit_insn (gen_one_cmpl<v_int_equiv>2 (operands[0],
>>> operands[0]));
>>> +      else if (code == UNEQ)
>>> +       {
>>> +         emit_insn (gen_aarch64_cmeq<mode> (tmp, operands[2],
>>> operands[3]));
>>> +         emit_insn (gen_orn<v_int_equiv>3 (operands[0], operands[0],
>>> tmp));
>>> +       }
>>>         break;
>>>         default:
>>> diff --git a/gcc/testsuite/gcc.target/aarch64/pr81647.c
>>> b/gcc/testsuite/gcc.target/aarch64/pr81647.c
>>> new file mode 100644
>>> index
>>> 0000000000000000000000000000000000000000..f60dfba49d538e3b2164b11392ab8dbfdba6546e
>>> --- /dev/null
>>> +++ b/gcc/testsuite/gcc.target/aarch64/pr81647.c
>>> @@ -0,0 +1,44 @@
>>> +/* { dg-do run } */
>>> +/* { dg-options "-O3 -fdump-tree-ssa" } */
>>> +
>>> +#include <fenv.h>
>>> +
>>> +double x[28], y[28];
>>> +int res[28];
>>> +
>>> +int
>>> +main (void)
>>> +{
>>> +  int i;
>>> +  for (i = 0; i < 28; ++i)
>>> +    {
>>> +      x[i] = __builtin_nan ("");
>>> +      y[i] = i;
>>> +    }
>>> +  __asm__ volatile ("" ::: "memory");
>>> +  feclearexcept (FE_ALL_EXCEPT);
>>> +  for (i = 0; i < 4; ++i)
>>> +    res[i] = __builtin_isgreater (x[i], y[i]);
>>> +  for (i = 4; i < 8; ++i)
>>> +    res[i] = __builtin_isgreaterequal (x[i], y[i]);
>>> +  for (i = 8; i < 12; ++i)
>>> +    res[i] = __builtin_isless (x[i], y[i]);
>>> +  for (i = 12; i < 16; ++i)
>>> +    res[i] = __builtin_islessequal (x[i], y[i]);
>>> +  for (i = 16; i < 20; ++i)
>>> +    res[i] = __builtin_islessgreater (x[i], y[i]);
>>> +  for (i = 20; i < 24; ++i)
>>> +    res[i] = __builtin_isunordered (x[i], y[i]);
>>> +  for (i = 24; i < 28; ++i)
>>> +    res[i] = !(__builtin_isunordered (x[i], y[i]));
>>> +  __asm__ volatile ("" ::: "memory");
>>> +  return fetestexcept (FE_ALL_EXCEPT) != 0;
>>> +}
>>> +
>>> +/* { dg-final { scan-tree-dump " u> " "ssa" } } */
>>> +/* { dg-final { scan-tree-dump " u>= " "ssa" } } */
>>> +/* { dg-final { scan-tree-dump " u< " "ssa" } } */
>>> +/* { dg-final { scan-tree-dump " u<= " "ssa" } } */
>>> +/* { dg-final { scan-tree-dump " u== " "ssa" } } */
>>> +/* { dg-final { scan-tree-dump " unord " "ssa" } } */
>>> +/* { dg-final { scan-tree-dump " ord " "ssa" } } */
>>
>>
>