From 865257c447cc50f5819e9b53da83145f3c36c305 Mon Sep 17 00:00:00 2001 From: Richard Sandiford Date: Thu, 16 Jan 2020 19:22:20 +0000 Subject: [PATCH] aarch64: Don't raise FE_INVALID for -__builtin_isgreater [PR93133] AIUI, the main purpose of REVERSE_CONDITION is to take advantage of any integer vs. FP information encoded in the CC mode, particularly when handling LT, LE, GE and GT. For integer comparisons we can safely map LT->GE, LE->GT, GE->LT and GT->LE, but for float comparisons this would usually be invalid without -ffinite-math-only. The aarch64 definition of REVERSE_CONDITION used reverse_condition_maybe_unordered for FP comparisons, which had the effect of converting an unordered-signalling LT, LE, GE or GT into a quiet UNGE, UNGT, UNLT or UNLE. And it would do the same in reverse: convert a quiet UN* into an unordered-signalling comparison. This would be safe in practice (although a little misleading) if we always used a compare:CCFP or compare:CCFPE to do the comparison and then used (gt (reg:CCFP/CCFPE CC_REGNUM) (const_int 0)) etc. to test the result. In that case any signal is raised by the compare and the choice of quiet vs. signalling relations doesn't matter when testing the result. The problem is that we also want to use GT directly on float registers, where any signal is raised by the comparison operation itself and so must follow the normal rtl rules (GT signalling, UNLE quiet). I think the safest fix is to make REVERSIBLE_CC_MODE return false for FP comparisons. We can then use the default REVERSE_CONDITION for integer comparisons and the usual conservatively-correct reversed_comparison_code_parts behaviour for FP comparisons. Unfortunately reversed_comparison_code_parts doesn't yet handle -ffinite-math-only, but that's probably GCC 11 material. A downside is that: int f (float x, float y) { return !(x < y); } now generates: fcmpe s0, s1 cset w0, mi eor w0, w0, 1 ret without -ffinite-math-only. Maybe for GCC 11 we should define rtx codes for all IEEE comparisons, so that we don't have this kind of representational gap. Changing REVERSE_CONDITION itself is pretty easy. However, the macro was also used in the ccmp handling, which relied on being able to reverse all comparisons. The patch adds new reversed patterns for cases in which the original condition needs to be kept. The test is based on gcc.dg/torture/pr91323.c. It might well fail on other targets that have similar bugs; please XFAIL as appropriate if you don't want to fix the target for GCC 10. 2020-01-17 Richard Sandiford gcc/ * config/aarch64/aarch64.h (REVERSIBLE_CC_MODE): Return false for FP modes. (REVERSE_CONDITION): Delete. * config/aarch64/iterators.md (CC_ONLY): New mode iterator. (CCFP_CCFPE): Likewise. (e): New mode attribute. * config/aarch64/aarch64.md (ccmp): Rename to... (@ccmp): ...this, using CC_ONLY instead of CC. (fccmp, fccmpe): Merge into... (@ccmp): ...this combined pattern. (@ccmp_rev): New pattern. (@ccmp_rev): Likewise. * config/aarch64/aarch64.c (aarch64_gen_compare_reg): Update name of generator from gen_ccmpdi to gen_ccmpccdi. (aarch64_gen_ccmp_next): Use code_for_ccmp. If we want to reverse the previous comparison but aren't able to, use the new ccmp_rev patterns instead. --- gcc/ChangeLog | 20 ++++++++ gcc/config/aarch64/aarch64.c | 30 ++++++++---- gcc/config/aarch64/aarch64.h | 10 ++-- gcc/config/aarch64/aarch64.md | 64 ++++++++++++++++++-------- gcc/config/aarch64/iterators.md | 6 +++ gcc/testsuite/gcc.dg/torture/pr93133.c | 41 +++++++++++++++++ 6 files changed, 136 insertions(+), 35 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/torture/pr93133.c diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 423899d3988..6c6d586ca75 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,3 +1,23 @@ +2020-01-17 Richard Sandiford + + * config/aarch64/aarch64.h (REVERSIBLE_CC_MODE): Return false + for FP modes. + (REVERSE_CONDITION): Delete. + * config/aarch64/iterators.md (CC_ONLY): New mode iterator. + (CCFP_CCFPE): Likewise. + (e): New mode attribute. + * config/aarch64/aarch64.md (ccmp): Rename to... + (@ccmp): ...this, using CC_ONLY instead of CC. + (fccmp, fccmpe): Merge into... + (@ccmp): ...this combined pattern. + (@ccmp_rev): New pattern. + (@ccmp_rev): Likewise. + * config/aarch64/aarch64.c (aarch64_gen_compare_reg): Update + name of generator from gen_ccmpdi to gen_ccmpccdi. + (aarch64_gen_ccmp_next): Use code_for_ccmp. If we want to reverse + the previous comparison but aren't able to, use the new ccmp_rev + patterns instead. + 2020-01-17 Richard Sandiford * gimplify.c (gimplify_return_expr): Use poly_int_tree_p rather diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index 600a238c1f4..3660ce7bde0 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -2345,9 +2345,9 @@ aarch64_gen_compare_reg (RTX_CODE code, rtx x, rtx y) rtx x_hi = operand_subword (x, 1, 0, TImode); rtx y_hi = operand_subword (y, 1, 0, TImode); - emit_insn (gen_ccmpdi (cc_reg, cc_reg, x_hi, y_hi, - gen_rtx_EQ (cc_mode, cc_reg, const0_rtx), - GEN_INT (AARCH64_EQ))); + emit_insn (gen_ccmpccdi (cc_reg, cc_reg, x_hi, y_hi, + gen_rtx_EQ (cc_mode, cc_reg, const0_rtx), + GEN_INT (AARCH64_EQ))); } else { @@ -20270,24 +20270,20 @@ aarch64_gen_ccmp_next (rtx_insn **prep_seq, rtx_insn **gen_seq, rtx prev, case E_HImode: case E_SImode: cmp_mode = SImode; - icode = CODE_FOR_ccmpsi; break; case E_DImode: cmp_mode = DImode; - icode = CODE_FOR_ccmpdi; break; case E_SFmode: cmp_mode = SFmode; cc_mode = aarch64_select_cc_mode ((rtx_code) cmp_code, op0, op1); - icode = cc_mode == CCFPEmode ? CODE_FOR_fccmpesf : CODE_FOR_fccmpsf; break; case E_DFmode: cmp_mode = DFmode; cc_mode = aarch64_select_cc_mode ((rtx_code) cmp_code, op0, op1); - icode = cc_mode == CCFPEmode ? CODE_FOR_fccmpedf : CODE_FOR_fccmpdf; break; default: @@ -20295,6 +20291,8 @@ aarch64_gen_ccmp_next (rtx_insn **prep_seq, rtx_insn **gen_seq, rtx prev, return NULL_RTX; } + icode = code_for_ccmp (cc_mode, cmp_mode); + op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode, unsignedp); op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode, unsignedp); if (!op0 || !op1) @@ -20310,9 +20308,21 @@ aarch64_gen_ccmp_next (rtx_insn **prep_seq, rtx_insn **gen_seq, rtx prev, if (bit_code != AND) { - prev = gen_rtx_fmt_ee (REVERSE_CONDITION (GET_CODE (prev), - GET_MODE (XEXP (prev, 0))), - VOIDmode, XEXP (prev, 0), const0_rtx); + /* Treat the ccmp patterns as canonical and use them where possible, + but fall back to ccmp_rev patterns if there's no other option. */ + rtx_code prev_code = GET_CODE (prev); + machine_mode prev_mode = GET_MODE (XEXP (prev, 0)); + if ((prev_mode == CCFPmode || prev_mode == CCFPEmode) + && !(prev_code == EQ + || prev_code == NE + || prev_code == ORDERED + || prev_code == UNORDERED)) + icode = code_for_ccmp_rev (cc_mode, cmp_mode); + else + { + rtx_code code = reverse_condition (prev_code); + prev = gen_rtx_fmt_ee (code, VOIDmode, XEXP (prev, 0), const0_rtx); + } aarch64_cond = AARCH64_INVERSE_CONDITION_CODE (aarch64_cond); } diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h index eb1eca4b2fe..f46b01c0ff8 100644 --- a/gcc/config/aarch64/aarch64.h +++ b/gcc/config/aarch64/aarch64.h @@ -1026,12 +1026,10 @@ typedef struct #define SELECT_CC_MODE(OP, X, Y) aarch64_select_cc_mode (OP, X, Y) -#define REVERSIBLE_CC_MODE(MODE) 1 - -#define REVERSE_CONDITION(CODE, MODE) \ - (((MODE) == CCFPmode || (MODE) == CCFPEmode) \ - ? reverse_condition_maybe_unordered (CODE) \ - : reverse_condition (CODE)) +/* Having an integer comparison mode guarantees that we can use + reverse_condition, but the usual restrictions apply to floating-point + comparisons. */ +#define REVERSIBLE_CC_MODE(MODE) ((MODE) != CCFPmode && (MODE) != CCFPEmode) #define CLZ_DEFINED_VALUE_AT_ZERO(MODE, VALUE) \ ((VALUE) = GET_MODE_UNIT_BITSIZE (MODE), 2) diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index c0b7010bcd3..41dc4ed4bc4 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -493,16 +493,18 @@ "" "") -(define_insn "ccmp" - [(set (match_operand:CC 1 "cc_register" "") - (if_then_else:CC +(define_insn "@ccmp" + [(set (match_operand:CC_ONLY 1 "cc_register" "") + (if_then_else:CC_ONLY (match_operator 4 "aarch64_comparison_operator" [(match_operand 0 "cc_register" "") (const_int 0)]) - (compare:CC + (compare:CC_ONLY (match_operand:GPI 2 "register_operand" "r,r,r") (match_operand:GPI 3 "aarch64_ccmp_operand" "r,Uss,Usn")) - (unspec:CC [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))] + (unspec:CC_ONLY + [(match_operand 5 "immediate_operand")] + UNSPEC_NZCV)))] "" "@ ccmp\\t%2, %3, %k5, %m4 @@ -511,33 +513,57 @@ [(set_attr "type" "alus_sreg,alus_imm,alus_imm")] ) -(define_insn "fccmp" - [(set (match_operand:CCFP 1 "cc_register" "") - (if_then_else:CCFP +(define_insn "@ccmp" + [(set (match_operand:CCFP_CCFPE 1 "cc_register" "") + (if_then_else:CCFP_CCFPE (match_operator 4 "aarch64_comparison_operator" [(match_operand 0 "cc_register" "") (const_int 0)]) - (compare:CCFP + (compare:CCFP_CCFPE (match_operand:GPF 2 "register_operand" "w") (match_operand:GPF 3 "register_operand" "w")) - (unspec:CCFP [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))] + (unspec:CCFP_CCFPE + [(match_operand 5 "immediate_operand")] + UNSPEC_NZCV)))] "TARGET_FLOAT" - "fccmp\\t%2, %3, %k5, %m4" + "fccmp\\t%2, %3, %k5, %m4" [(set_attr "type" "fccmp")] ) -(define_insn "fccmpe" - [(set (match_operand:CCFPE 1 "cc_register" "") - (if_then_else:CCFPE +(define_insn "@ccmp_rev" + [(set (match_operand:CC_ONLY 1 "cc_register" "") + (if_then_else:CC_ONLY (match_operator 4 "aarch64_comparison_operator" [(match_operand 0 "cc_register" "") - (const_int 0)]) - (compare:CCFPE + (const_int 0)]) + (unspec:CC_ONLY + [(match_operand 5 "immediate_operand")] + UNSPEC_NZCV) + (compare:CC_ONLY + (match_operand:GPI 2 "register_operand" "r,r,r") + (match_operand:GPI 3 "aarch64_ccmp_operand" "r,Uss,Usn"))))] + "" + "@ + ccmp\\t%2, %3, %k5, %M4 + ccmp\\t%2, %3, %k5, %M4 + ccmn\\t%2, #%n3, %k5, %M4" + [(set_attr "type" "alus_sreg,alus_imm,alus_imm")] +) + +(define_insn "@ccmp_rev" + [(set (match_operand:CCFP_CCFPE 1 "cc_register" "") + (if_then_else:CCFP_CCFPE + (match_operator 4 "aarch64_comparison_operator" + [(match_operand 0 "cc_register" "") + (const_int 0)]) + (unspec:CCFP_CCFPE + [(match_operand 5 "immediate_operand")] + UNSPEC_NZCV) + (compare:CCFP_CCFPE (match_operand:GPF 2 "register_operand" "w") - (match_operand:GPF 3 "register_operand" "w")) - (unspec:CCFPE [(match_operand 5 "immediate_operand")] UNSPEC_NZCV)))] + (match_operand:GPF 3 "register_operand" "w"))))] "TARGET_FLOAT" - "fccmpe\\t%2, %3, %k5, %m4" + "fccmp\\t%2, %3, %k5, %M4" [(set_attr "type" "fccmp")] ) diff --git a/gcc/config/aarch64/iterators.md b/gcc/config/aarch64/iterators.md index 661c3e7b4a7..fc973086cb9 100644 --- a/gcc/config/aarch64/iterators.md +++ b/gcc/config/aarch64/iterators.md @@ -22,6 +22,9 @@ ;; Mode Iterators ;; ------------------------------------------------------------------- +;; Condition-code iterators. +(define_mode_iterator CC_ONLY [CC]) +(define_mode_iterator CCFP_CCFPE [CCFP CCFPE]) ;; Iterator for General Purpose Integer registers (32- and 64-bit modes) (define_mode_iterator GPI [SI DI]) @@ -833,6 +836,9 @@ ;; Mode attributes ;; ------------------------------------------------------------------- +;; "e" for signaling operations, "" for quiet operations. +(define_mode_attr e [(CCFP "") (CCFPE "e")]) + ;; In GPI templates, a string like "%0" will expand to "%w0" in the ;; 32-bit version and "%x0" in the 64-bit version. (define_mode_attr w [(QI "w") (HI "w") (SI "w") (DI "x") (SF "s") (DF "d")]) diff --git a/gcc/testsuite/gcc.dg/torture/pr93133.c b/gcc/testsuite/gcc.dg/torture/pr93133.c new file mode 100644 index 00000000000..21eae1eb3dd --- /dev/null +++ b/gcc/testsuite/gcc.dg/torture/pr93133.c @@ -0,0 +1,41 @@ +/* { dg-do run } */ +/* { dg-add-options ieee } */ +/* { dg-require-effective-target fenv_exceptions } */ +/* { dg-skip-if "fenv" { powerpc-ibm-aix* } } */ + +#include + +int +__attribute__ ((noinline, noclone)) +f1 (float a, float b) +{ + return -__builtin_isgreater (a, b); +} + +int +__attribute__ ((noinline, noclone)) +f2 (float a, float b) +{ + return -(a > b); +} + +int +main (void) +{ + volatile int r; + + float nanf = __builtin_nanf (""); + float argf = 1.0f; + + feclearexcept (FE_INVALID); + + r = f1 (nanf, argf); + if (r != 0 || fetestexcept (FE_INVALID)) + __builtin_abort (); + + r = f2 (nanf, argf); + if (r != 0 || !fetestexcept (FE_INVALID)) + __builtin_abort (); + + return 0; +} -- 2.30.2