public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
@ 2020-09-04 15:52 Raoni Fassina Firmino
  2020-09-18 17:32 ` Raoni Fassina Firmino
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Raoni Fassina Firmino @ 2020-09-04 15:52 UTC (permalink / raw)
  To: gcc-patches; +Cc: segher, joseph

Changes since v1[1]:
  - Fixed english spelling;
  - Fixed code-style;
  - Changed match operand predicate in feclearexcept and feraiseexcept;
  - Changed testcase options;
  - Minor changes in test code to be C90 compatible;
  - Other minor changes sugested by Segher;
  - Changed subject line tag (not sure if I tagged correctly or should
    include optabs: also)

There is one pending question raised by Segher, It is about adding
documentation, I am not sure if it is needed and if so, where it
should be. I will quote the relevant part of the conversation[2] from
the v1 thread for context:

  > > > +OPTAB_D (fegetround_optab, "fegetround$a")
  > > > +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
  > > > +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
  > >␣
  > > Should those be documented somewhere?  (In gcc/doc/ somewhere).
  >
  > I am lost on that one. I took a look on the docs (I hope looking on the
  > online docs was good enough) and I didn't find a place where i feel it
  > sits well. On the PowerPC target specific sections (6.60.22 Basic
  > PowerPC Built-in Functions), I didn't found it mentioning builtins that
  > are optimizations for the standard library functions, but we do have
  > many of these for Power.  Then, on the generic section (6.59 Other
  > Built-in Functions Provided by GCC) it mentions C99 functions that have
  > builtins but it seems like it mentions builtins that have target
  > independent implementation, or at least it dos not say that some
  > builtins may be implemented on only some targets.  And in this case
  > there is no implementation (for now) for any other target that is not
  > PowerPc.
  >
  > So, I don't know if or where this should be documented.

tested on top of master (c5a6c2237a1156dc43fa32c938fc32acdfc2bff9)
on the following plataforms with no regression:
  - powerpc64le-linux-gnu (Power 9)
  - powerpc64le-linux-gnu (Power 8)

[1] https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552024.html
[2] https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553295.html

---- 8< ----

This optimizations were originally in glibc, but was removed
and sugested that they were a good fit as gcc builtins[1].

The associated bugreport: PR target/94193

[1] https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00047.html
    https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00080.html

2020-08-13  Raoni Fassina Firmino  <raoni@linux.ibm.com>

gcc/ChangeLog:

	* builtins.c (expand_builtin_fegetround): New function.
	(expand_builtin_feclear_feraise_except): New function.
	(expand_builtin): Add cases for BUILT_IN_FEGETROUND,
	BUILT_IN_FECLEAREXCEPT and BUILT_IN_FERAISEEXCEPT
	* config/rs6000/rs6000.md (fegetroundsi): New pattern.
	(feclearexceptsi): New Pattern.
	(feraiseexceptsi): New Pattern.
	* optabs.def (fegetround_optab): New optab.
	(feclearexcept_optab): New optab.
	(feraiseexcept_optab): New optab.

gcc/testsuite/ChangeLog:

	* gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: New test.
	* gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: New test.
	* gcc.target/powerpc/builtin-fegetround.c: New test.

Signed-off-by: Raoni Fassina Firmino <raoni@linux.ibm.com>

FIX: patch_v1 (2)
---
 gcc/builtins.c                                |  76 ++++++++++
 gcc/config/rs6000/rs6000.md                   |  82 +++++++++++
 gcc/optabs.def                                |   4 +
 .../builtin-feclearexcept-feraiseexcept-1.c   |  68 +++++++++
 .../builtin-feclearexcept-feraiseexcept-2.c   | 133 ++++++++++++++++++
 .../gcc.target/powerpc/builtin-fegetround.c   |  36 +++++
 6 files changed, 399 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c

diff --git a/gcc/builtins.c b/gcc/builtins.c
index 97f1a184dc6..a6f6141edb7 100644
--- a/gcc/builtins.c
+++ b/gcc/builtins.c
@@ -115,6 +115,9 @@ static rtx expand_builtin_mathfn_3 (tree, rtx, rtx);
 static rtx expand_builtin_mathfn_ternary (tree, rtx, rtx);
 static rtx expand_builtin_interclass_mathfn (tree, rtx);
 static rtx expand_builtin_sincos (tree);
+static rtx expand_builtin_fegetround (tree, rtx, machine_mode);
+static rtx expand_builtin_feclear_feraise_except (tree, rtx, machine_mode,
+						  optab);
 static rtx expand_builtin_cexpi (tree, rtx);
 static rtx expand_builtin_int_roundingfn (tree, rtx);
 static rtx expand_builtin_int_roundingfn_2 (tree, rtx);
@@ -2701,6 +2704,59 @@ expand_builtin_sincos (tree exp)
   return const0_rtx;
 }
 
+/* Expand call EXP to the fegetround builtin (from C99 venv.h), returning the
+   result and setting it in TARGET.  Otherwise return NULL_RTX on failure.  */
+static rtx
+expand_builtin_fegetround (tree exp, rtx target, machine_mode target_mode)
+{
+  if (!validate_arglist (exp, VOID_TYPE))
+    return NULL_RTX;
+
+  insn_code icode = direct_optab_handler (fegetround_optab, SImode);
+  if (icode == CODE_FOR_nothing)
+    return NULL_RTX;
+
+  if (target == 0
+      || GET_MODE (target) != target_mode
+      || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
+    target = gen_reg_rtx (target_mode);
+
+  rtx pat = GEN_FCN (icode) (target);
+  if (! pat)
+    return NULL_RTX;
+  emit_insn (pat);
+
+  return target;
+}
+
+/* Expand call EXP to either feclearexcept or feraiseexcept builtins (from C99
+    fenv.h), returning the result and setting it in TARGET.  Otherwise return
+    NULL_RTX on failure.  */
+static rtx
+expand_builtin_feclear_feraise_except (tree exp, rtx target,
+				       machine_mode target_mode, optab op_optab)
+{
+  if (!validate_arglist (exp, INTEGER_TYPE, VOID_TYPE))
+    return NULL_RTX;
+  rtx op0 = expand_normal (CALL_EXPR_ARG (exp, 0));
+
+  insn_code icode = direct_optab_handler (op_optab, SImode);
+  if (icode == CODE_FOR_nothing)
+    return NULL_RTX;
+
+  if (target == 0
+      || GET_MODE (target) != target_mode
+      || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
+    target = gen_reg_rtx (target_mode);
+
+  rtx pat = GEN_FCN (icode) (target, op0);
+  if (!pat)
+    return NULL_RTX;
+  emit_insn (pat);
+
+  return target;
+}
+
 /* Expand a call to the internal cexpi builtin to the sincos math function.
    EXP is the expression that is a call to the builtin function; if convenient,
    the result should be placed in TARGET.  */
@@ -8290,6 +8346,26 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
 	return target;
       break;
 
+    case BUILT_IN_FEGETROUND:
+      target = expand_builtin_fegetround (exp, target, target_mode);
+      if (target)
+	return target;
+      break;
+
+    case BUILT_IN_FECLEAREXCEPT:
+      target = expand_builtin_feclear_feraise_except (exp, target, target_mode,
+						      feclearexcept_optab);
+      if (target)
+	return target;
+      break;
+
+    case BUILT_IN_FERAISEEXCEPT:
+      target = expand_builtin_feclear_feraise_except (exp, target, target_mode,
+						      feraiseexcept_optab);
+      if (target)
+	return target;
+      break;
+
     case BUILT_IN_APPLY_ARGS:
       return expand_builtin_apply_args ();
 
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index 43b620ae1c0..c19908040d1 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -6565,6 +6565,88 @@
   [(set_attr "type" "fpload")
    (set_attr "length" "8")
    (set_attr "isa" "*,p8v,p8v")])
+
+;; int __builtin_fegetround()
+(define_expand "fegetroundsi"
+  [(use (match_operand:SI 0 "gpc_reg_operand"))]
+  "TARGET_HARD_FLOAT"
+{
+  rtx tmp_df = gen_reg_rtx (DFmode);
+  emit_insn (gen_rs6000_mffsl (tmp_df));
+
+  rtx tmp_di = simplify_gen_subreg (DImode, tmp_df, DFmode, 0);
+  rtx tmp_di_2 = gen_reg_rtx (DImode);
+  emit_insn (gen_anddi3 (tmp_di_2, tmp_di, GEN_INT (3)));
+  rtx tmp_si = gen_reg_rtx (SImode);
+  tmp_si = simplify_gen_subreg (SImode, tmp_di_2, DImode, 0);
+  emit_move_insn (operands[0], tmp_si);
+  DONE;
+})
+
+;; int feclearexcept(int excepts)
+;;
+;; This expansion for the C99 function only works when excepts is a
+;; constant know at compile time and specifying only one of
+;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
+;; It doesn't handle values out of range, and always returns 0.
+;; Note that FE_INVALID is unsuported because it maps to more than
+;; one bit on FPSCR register.
+;; Because of these restrictions, this only expands on the desired cases.
+(define_expand "feclearexceptsi"
+  [(use (match_operand:SI 1 "const_int_operand" "n"))
+   (set (match_operand:SI 0 "gpc_reg_operand")
+	(const_int 0))]
+  "TARGET_HARD_FLOAT"
+{
+  switch (INTVAL (operands[1]))
+    {
+    case 0x2000000:  /* FE_INEXACT */
+    case 0x4000000:  /* FE_DIVBYZERO */
+    case 0x8000000:  /* FE_UNDERFLOW */
+    case 0x10000000: /* FE_OVERFLOW */
+      break;
+    default:
+      FAIL;
+    }
+
+  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
+  emit_insn (gen_rs6000_mtfsb0 (tmp));
+  emit_move_insn (operands[0], const0_rtx);
+  DONE;
+})
+
+;; int fegraiseexcept(int excepts)
+;;
+;; This expansion for the C99 function only works when excepts is a
+;; constant know at compile time and specifying only one of
+;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
+;; It doesn't handle values out of range, and always returns 0.
+;; Note that FE_INVALID is unsupported because it maps to more than
+;; one bit on FPSCR register.
+;; Because of these restrictions, this only expands on the desired cases.
+(define_expand "feraiseexceptsi"
+  [(use (match_operand:SI 1 "const_int_operand" "n"))
+   (set (match_operand:SI 0 "gpc_reg_operand")
+	(const_int 0))]
+  "TARGET_HARD_FLOAT"
+{
+  switch (INTVAL (operands[1]))
+    {
+    case 0x2000000:  /* FE_INEXACT */
+    case 0x4000000:  /* FE_DIVBYZERO */
+    case 0x8000000:  /* FE_UNDERFLOW */
+    case 0x10000000: /* FE_OVERFLOW */
+      break;
+    default:
+      FAIL;
+    }
+
+  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
+  emit_insn (gen_rs6000_mtfsb1 (tmp));
+  emit_move_insn (operands[0], const0_rtx);
+  DONE;
+})
+
 \f
 ;; Define the TImode operations that can be done in a small number
 ;; of instructions.  The & constraints are to prevent the register
diff --git a/gcc/optabs.def b/gcc/optabs.def
index 78409aa1453..987ee0f79dc 100644
--- a/gcc/optabs.def
+++ b/gcc/optabs.def
@@ -318,6 +318,10 @@ OPTAB_D (sinh_optab, "sinh$a2")
 OPTAB_D (tan_optab, "tan$a2")
 OPTAB_D (tanh_optab, "tanh$a2")
 
+OPTAB_D (fegetround_optab, "fegetround$a")
+OPTAB_D (feclearexcept_optab, "feclearexcept$a")
+OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
+
 /* C99 implementations of fmax/fmin.  */
 OPTAB_D (fmax_optab, "fmax$a3")
 OPTAB_D (fmin_optab, "fmin$a3")
diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
new file mode 100644
index 00000000000..a2977d188e5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
@@ -0,0 +1,68 @@
+/* { dg-do run } */
+/* { dg-require-effective-target fenv_exceptions } */
+/* { dg-options "-lm -fno-builtin" } */
+
+/* This testcase ensures that the builtins expand with the matching arguments
+ * or otherwise fallback gracefull to a function call, and don't ICE during
+ * compilation. */
+
+#include <fenv.h>
+
+/* We use __builtin_* version to avoid the function be replaced by glibc that
+ * may have an inline optimization for it in fenv.h. */
+int
+main ()
+{
+  int   rsi = 0;
+  long  rsl = 0;
+  short rss = 0;
+  char  rsc = 0;
+
+  unsigned int   rui = 0;
+  unsigned long  rul = 0;
+  unsigned short rus = 0;
+  unsigned char  ruc = 0;
+
+  int e = FE_DIVBYZERO;
+
+  __builtin_feclearexcept(e);                          // CALL
+  __builtin_feclearexcept(0);                          // CALL
+  __builtin_feclearexcept(FE_ALL_EXCEPT);              // CALL
+  __builtin_feclearexcept(FE_INVALID);                 // CALL
+  __builtin_feclearexcept(FE_INEXACT | FE_DIVBYZERO);  // CALL
+  __builtin_feclearexcept(FE_INEXACT);    // EXPAND
+  __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  __builtin_feclearexcept(FE_UNDERFLOW);  // EXPAND
+  __builtin_feclearexcept(FE_OVERFLOW);   // EXPAND
+
+  rsi = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  rsl = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  rss = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  rsc = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  rui = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  rul = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  rus = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+  ruc = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
+
+
+  __builtin_feraiseexcept(e);                          // CALL
+  __builtin_feraiseexcept(0);                          // CALL
+  __builtin_feraiseexcept(FE_ALL_EXCEPT);              // CALL
+  __builtin_feraiseexcept(FE_INVALID);                 // CALL
+  __builtin_feraiseexcept(FE_INEXACT | FE_DIVBYZERO);  // CALL
+  __builtin_feraiseexcept(FE_INEXACT);    // EXPAND
+  __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  __builtin_feraiseexcept(FE_UNDERFLOW);  // EXPAND
+  __builtin_feraiseexcept(FE_OVERFLOW);   // EXPAND
+
+  rsi = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  rsl = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  rss = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  rsc = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  rui = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  rul = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  rus = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+  ruc = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
new file mode 100644
index 00000000000..4eb2a63d48f
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
@@ -0,0 +1,133 @@
+/* { dg-do run } */
+/* { dg-require-effective-target fenv_exceptions } */
+/* { dg-options "-lm -fno-builtin" } */
+
+/* This testcase ensures that the builtins are correctly expanded and match the
+ * expected result. */
+
+#include <fenv.h>
+
+#ifdef DEBUG
+#include <stdio.h>
+#define INFO(...) printf(__VA_ARGS__)
+#define FAIL(v, e, x, s, f) \
+        printf("ERROR [l %d] testing %s(%x): %s returned %x," \
+               " expecected %x\n", __LINE__, s, x, f, v, e)
+#else
+void abort (void);
+#define INFO(...)
+#define FAIL(v, e, x, s, f) abort()
+#endif
+
+/* We use __builtin_* version to avoid the function be replaced by glibc that
+ * may have an inline optimization for it in fenv.h. */
+int
+main ()
+{
+  char *s = 0;
+  int e = 0;
+  int raised = 0;
+
+  s = "FE_ALL_EXCEPT";
+  e = FE_ALL_EXCEPT;
+  INFO("test: %s(%x)\n", s, e);
+
+  feclearexcept(FE_ALL_EXCEPT);
+  __builtin_feraiseexcept(FE_ALL_EXCEPT);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised != e)
+    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
+
+  feraiseexcept(FE_ALL_EXCEPT);
+  __builtin_feclearexcept(FE_ALL_EXCEPT);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised == FE_ALL_EXCEPT & ~e)
+    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
+
+
+  s = "FE_DIVBYZERO";
+  e = FE_DIVBYZERO;
+  INFO("test: %s(%x)\n", s, e);
+
+  feclearexcept(FE_ALL_EXCEPT);
+  __builtin_feraiseexcept(FE_DIVBYZERO);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised != e)
+    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
+
+  feraiseexcept(FE_ALL_EXCEPT);
+  __builtin_feclearexcept(FE_DIVBYZERO);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised == FE_ALL_EXCEPT & ~e)
+    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
+
+
+  s = "FE_INEXACT";
+  e = FE_INEXACT;
+  INFO("test: %s(%x)\n", s, e);
+
+  feclearexcept(FE_ALL_EXCEPT);
+  __builtin_feraiseexcept(FE_INEXACT);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised != e)
+    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
+
+  feraiseexcept(FE_ALL_EXCEPT);
+  __builtin_feclearexcept(FE_INEXACT);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised == FE_ALL_EXCEPT & ~e)
+    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
+
+
+  s = "FE_OVERFLOW";
+  e = FE_OVERFLOW;
+  INFO("test: %s(%x)\n", s, e);
+
+  feclearexcept(FE_ALL_EXCEPT);
+  __builtin_feraiseexcept(FE_OVERFLOW);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised != e)
+    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
+
+  feraiseexcept(FE_ALL_EXCEPT);
+  __builtin_feclearexcept(FE_OVERFLOW);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised == FE_ALL_EXCEPT & ~e)
+    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
+
+
+  s = "FE_UNDERFLOW";
+  e = FE_UNDERFLOW;
+  INFO("test: %s(%x)\n", s, e);
+
+  feclearexcept(FE_ALL_EXCEPT);
+  __builtin_feraiseexcept(FE_UNDERFLOW);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised != e)
+    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
+
+  feraiseexcept(FE_ALL_EXCEPT);
+  __builtin_feclearexcept(FE_UNDERFLOW);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised == FE_ALL_EXCEPT & ~e)
+    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
+
+
+  s = "FE_INVALID";
+  e = FE_INVALID;
+  INFO("test: %s(%x)\n", s, e);
+
+  feclearexcept(FE_ALL_EXCEPT);
+  __builtin_feraiseexcept(FE_INVALID);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised != e)
+    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
+
+  feraiseexcept(FE_ALL_EXCEPT);
+  __builtin_feclearexcept(FE_INVALID);
+  raised = fetestexcept(FE_ALL_EXCEPT);
+  if (raised == FE_ALL_EXCEPT & ~e)
+    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
+
+  return 0;
+}
diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c b/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c
new file mode 100644
index 00000000000..c3649f874a5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c
@@ -0,0 +1,36 @@
+/* { dg-do run } */
+/* { dg-require-effective-target fenv_exceptions } */
+/* { dg-options "-lm -fno-builtin" } */
+
+/* This testcase ensures that the builtins is correctly expanded and match the
+ * expected result from the standard function. */
+
+#include <fenv.h>
+
+#ifdef DEBUG
+#include <stdio.h>
+#define FAIL(v, e) printf("ERROR, __builtin_fegetround() returned %d," \
+                          " not the expecected value %d\n", v, e);
+#else
+void abort (void);
+#define FAIL(v, e) abort()
+#endif
+
+/* We use __builtin_* version to avoid the function be replaced by glibc that
+ * may have an inline optimization for it in fenv.h. */
+int
+main ()
+{
+  int i, rounding, expected;
+  const int rm[] = {FE_TONEAREST, FE_TOWARDZERO, FE_UPWARD, FE_DOWNWARD};
+  for (i = 0; i < sizeof(rm); i++)
+    {
+      fesetround(rm[i]);
+      rounding = __builtin_fegetround();
+      expected = fegetround();
+      if (rounding != expected)
+        FAIL(rounding, expected);
+    }
+
+  return 0;
+}
-- 
2.26.2


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-04 15:52 [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193] Raoni Fassina Firmino
@ 2020-09-18 17:32 ` Raoni Fassina Firmino
  2020-09-28 12:41   ` Raoni Fassina Firmino
  2020-09-28 16:42 ` will schmidt
  2020-09-29 23:38 ` [PATCH v2] builtins: (not just) " Segher Boessenkool
  2 siblings, 1 reply; 16+ messages in thread
From: Raoni Fassina Firmino @ 2020-09-18 17:32 UTC (permalink / raw)
  To: gcc-patches, segher, joseph

Ping.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-18 17:32 ` Raoni Fassina Firmino
@ 2020-09-28 12:41   ` Raoni Fassina Firmino
  0 siblings, 0 replies; 16+ messages in thread
From: Raoni Fassina Firmino @ 2020-09-28 12:41 UTC (permalink / raw)
  To: gcc-patches, segher, joseph

Ping.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-04 15:52 [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193] Raoni Fassina Firmino
  2020-09-18 17:32 ` Raoni Fassina Firmino
@ 2020-09-28 16:42 ` will schmidt
  2020-10-05  1:56   ` Hans-Peter Nilsson
  2020-10-26 16:05   ` Raoni Fassina Firmino
  2020-09-29 23:38 ` [PATCH v2] builtins: (not just) " Segher Boessenkool
  2 siblings, 2 replies; 16+ messages in thread
From: will schmidt @ 2020-09-28 16:42 UTC (permalink / raw)
  To: Raoni Fassina Firmino, gcc-patches; +Cc: joseph, segher

On Fri, 2020-09-04 at 12:52 -0300, Raoni Fassina Firmino via Gcc-patches wrote:
> Changes since v1[1]:
>   - Fixed english spelling;
>   - Fixed code-style;
>   - Changed match operand predicate in feclearexcept and feraiseexcept;
>   - Changed testcase options;
>   - Minor changes in test code to be C90 compatible;
>   - Other minor changes sugested by Segher;
>   - Changed subject line tag (not sure if I tagged correctly or should
>     include optabs: also)
> 
> There is one pending question raised by Segher, It is about adding
> documentation, I am not sure if it is needed and if so, where it
> should be. I will quote the relevant part of the conversation[2] from
> the v1 thread for context:
> 
>   > > > +OPTAB_D (fegetround_optab, "fegetround$a")
>   > > > +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
>   > > > +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
>   > >␣
>   > > Should those be documented somewhere?  (In gcc/doc/ somewhere).
>   >
>   > I am lost on that one. I took a look on the docs (I hope looking on the
>   > online docs was good enough) and I didn't find a place where i feel it
>   > sits well. On the PowerPC target specific sections (6.60.22 Basic
>   > PowerPC Built-in Functions), I didn't found it mentioning builtins that
>   > are optimizations for the standard library functions, but we do have
>   > many of these for Power.  Then, on the generic section (6.59 Other
>   > Built-in Functions Provided by GCC) it mentions C99 functions that have
>   > builtins but it seems like it mentions builtins that have target
>   > independent implementation, or at least it dos not say that some
>   > builtins may be implemented on only some targets.  And in this case
>   > there is no implementation (for now) for any other target that is not
>   > PowerPc.
>   >
>   > So, I don't know if or where this should be documented.
> 
> tested on top of master (c5a6c2237a1156dc43fa32c938fc32acdfc2bff9)
> on the following plataforms with no regression:
>   - powerpc64le-linux-gnu (Power 9)
>   - powerpc64le-linux-gnu (Power 8)
> 
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2020-August/552024.html
> [2] https://gcc.gnu.org/pipermail/gcc-patches/2020-September/553295.html
> 
> ---- 8< ----
> 
> This optimizations were originally in glibc, but was removed
> and sugested that they were a good fit as gcc builtins[1].
> 
> The associated bugreport: PR target/94193
> 
> [1] https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00047.html
>     https://sourceware.org/legacy-ml/libc-alpha/2020-03/msg00080.html
> 
> 2020-08-13  Raoni Fassina Firmino  <raoni@linux.ibm.com>
> 
> gcc/ChangeLog:
> 
> 	* builtins.c (expand_builtin_fegetround): New function.
> 	(expand_builtin_feclear_feraise_except): New function.
> 	(expand_builtin): Add cases for BUILT_IN_FEGETROUND,
> 	BUILT_IN_FECLEAREXCEPT and BUILT_IN_FERAISEEXCEPT
> 	* config/rs6000/rs6000.md (fegetroundsi): New pattern.
> 	(feclearexceptsi): New Pattern.
> 	(feraiseexceptsi): New Pattern.
> 	* optabs.def (fegetround_optab): New optab.
> 	(feclearexcept_optab): New optab.
> 	(feraiseexcept_optab): New optab.

> 
> gcc/testsuite/ChangeLog:
> 
> 	* gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c: New test.
> 	* gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c: New test.
> 	* gcc.target/powerpc/builtin-fegetround.c: New test.
> 

A few cosmetic nits below,.. this is perhaps enough activity to get
others to take a look.  :-) 


> Signed-off-by: Raoni Fassina Firmino <raoni@linux.ibm.com>
> 
> FIX: patch_v1 (2)
> ---
>  gcc/builtins.c                                |  76 ++++++++++
>  gcc/config/rs6000/rs6000.md                   |  82 +++++++++++
>  gcc/optabs.def                                |   4 +
>  .../builtin-feclearexcept-feraiseexcept-1.c   |  68 +++++++++
>  .../builtin-feclearexcept-feraiseexcept-2.c   | 133 ++++++++++++++++++
>  .../gcc.target/powerpc/builtin-fegetround.c   |  36 +++++
>  6 files changed, 399 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c
> 
> diff --git a/gcc/builtins.c b/gcc/builtins.c
> index 97f1a184dc6..a6f6141edb7 100644
> --- a/gcc/builtins.c
> +++ b/gcc/builtins.c
> @@ -115,6 +115,9 @@ static rtx expand_builtin_mathfn_3 (tree, rtx, rtx);
>  static rtx expand_builtin_mathfn_ternary (tree, rtx, rtx);
>  static rtx expand_builtin_interclass_mathfn (tree, rtx);
>  static rtx expand_builtin_sincos (tree);
> +static rtx expand_builtin_fegetround (tree, rtx, machine_mode);
> +static rtx expand_builtin_feclear_feraise_except (tree, rtx, machine_mode,
> +						  optab);


>  static rtx expand_builtin_cexpi (tree, rtx);
>  static rtx expand_builtin_int_roundingfn (tree, rtx);
>  static rtx expand_builtin_int_roundingfn_2 (tree, rtx);
> @@ -2701,6 +2704,59 @@ expand_builtin_sincos (tree exp)
>    return const0_rtx;
>  }
> 
> +/* Expand call EXP to the fegetround builtin (from C99 venv.h), returning the
> +   result and setting it in TARGET.  Otherwise return NULL_RTX on failure.  */
> +static rtx
> +expand_builtin_fegetround (tree exp, rtx target, machine_mode target_mode)
> +{
> +  if (!validate_arglist (exp, VOID_TYPE))
> +    return NULL_RTX;
> +
> +  insn_code icode = direct_optab_handler (fegetround_optab, SImode);
> +  if (icode == CODE_FOR_nothing)
> +    return NULL_RTX;
> +
> +  if (target == 0
> +      || GET_MODE (target) != target_mode
> +      || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
> +    target = gen_reg_rtx (target_mode);
> +
> +  rtx pat = GEN_FCN (icode) (target);
> +  if (! pat)
> +    return NULL_RTX;
> +  emit_insn (pat);
> +
> +  return target;
> +}
> +
> +/* Expand call EXP to either feclearexcept or feraiseexcept builtins (from C99
> +    fenv.h), returning the result and setting it in TARGET.  Otherwise return
> +    NULL_RTX on failure.  */
> +static rtx
> +expand_builtin_feclear_feraise_except (tree exp, rtx target,
> +				       machine_mode target_mode, optab op_optab)
> +{
> +  if (!validate_arglist (exp, INTEGER_TYPE, VOID_TYPE))
> +    return NULL_RTX;
> +  rtx op0 = expand_normal (CALL_EXPR_ARG (exp, 0));
> +
> +  insn_code icode = direct_optab_handler (op_optab, SImode);
> +  if (icode == CODE_FOR_nothing)
> +    return NULL_RTX;
> +
> +  if (target == 0
> +      || GET_MODE (target) != target_mode
> +      || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
> +    target = gen_reg_rtx (target_mode);
> +
> +  rtx pat = GEN_FCN (icode) (target, op0);
> +  if (!pat)
> +    return NULL_RTX;
> +  emit_insn (pat);
> +
> +  return target;
> +}


I don't see any references to feclearexcept or feraiseexcept in the
functions there.   I see the callers pass in those values via optab,
but nothing in these functions explicitly checks or limits that in a
way that is obvious upon my reading...  Thus I wonder if there may be a
different generic names that would be appropriate for the functions.



> +
>  /* Expand a call to the internal cexpi builtin to the sincos math function.
>     EXP is the expression that is a call to the builtin function; if convenient,
>     the result should be placed in TARGET.  */
> @@ -8290,6 +8346,26 @@ expand_builtin (tree exp, rtx target, rtx subtarget, machine_mode mode,
>  	return target;
>        break;
> 
> +    case BUILT_IN_FEGETROUND:
> +      target = expand_builtin_fegetround (exp, target, target_mode);
> +      if (target)
> +	return target;
> +      break;
> +
> +    case BUILT_IN_FECLEAREXCEPT:
> +      target = expand_builtin_feclear_feraise_except (exp, target, target_mode,
> +						      feclearexcept_optab);
> +      if (target)
> +	return target;
> +      break;
> +
> +    case BUILT_IN_FERAISEEXCEPT:
> +      target = expand_builtin_feclear_feraise_except (exp, target, target_mode,
> +						      feraiseexcept_optab);
> +      if (target)
> +	return target;
> +      break;
> +
>      case BUILT_IN_APPLY_ARGS:
>        return expand_builtin_apply_args ();
> 
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index 43b620ae1c0..c19908040d1 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -6565,6 +6565,88 @@
>    [(set_attr "type" "fpload")
>     (set_attr "length" "8")
>     (set_attr "isa" "*,p8v,p8v")])
> +
> +;; int __builtin_fegetround()
> +(define_expand "fegetroundsi"
> +  [(use (match_operand:SI 0 "gpc_reg_operand"))]
> +  "TARGET_HARD_FLOAT"
> +{
> +  rtx tmp_df = gen_reg_rtx (DFmode);
> +  emit_insn (gen_rs6000_mffsl (tmp_df));
> +
> +  rtx tmp_di = simplify_gen_subreg (DImode, tmp_df, DFmode, 0);
> +  rtx tmp_di_2 = gen_reg_rtx (DImode);
> +  emit_insn (gen_anddi3 (tmp_di_2, tmp_di, GEN_INT (3)));
> +  rtx tmp_si = gen_reg_rtx (SImode);
> +  tmp_si = simplify_gen_subreg (SImode, tmp_di_2, DImode, 0);
> +  emit_move_insn (operands[0], tmp_si);
> +  DONE;
> +})
> +
> +;; int feclearexcept(int excepts)
> +;;
> +;; This expansion for the C99 function only works when excepts is a
> +;; constant know at compile time and specifying only one of
known
specifies

> +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> +;; It doesn't handle values out of range, and always returns 0.
> +;; Note that FE_INVALID is unsuported because it maps to more than
> +;; one bit on FPSCR register.
> +;; Because of these restrictions, this only expands on the desired cases.
> +(define_expand "feclearexceptsi"
> +  [(use (match_operand:SI 1 "const_int_operand" "n"))
> +   (set (match_operand:SI 0 "gpc_reg_operand")
> +	(const_int 0))]
> +  "TARGET_HARD_FLOAT"
> +{
> +  switch (INTVAL (operands[1]))
> +    {
> +    case 0x2000000:  /* FE_INEXACT */
> +    case 0x4000000:  /* FE_DIVBYZERO */
> +    case 0x8000000:  /* FE_UNDERFLOW */
> +    case 0x10000000: /* FE_OVERFLOW */
> +      break;
> +    default:
> +      FAIL;
> +    }
> +
> +  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
> +  emit_insn (gen_rs6000_mtfsb0 (tmp));
> +  emit_move_insn (operands[0], const0_rtx);
> +  DONE;
> +})
> +
> +;; int fegraiseexcept(int excepts)
> +;;
> +;; This expansion for the C99 function only works when excepts is a
> +;; constant know at compile time and specifying only one of
known
specifies

> +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> +;; It doesn't handle values out of range, and always returns 0.
> +;; Note that FE_INVALID is unsupported because it maps to more than
> +;; one bit on FPSCR register.

Should FE_INVALID have an explicit case statement path to FAIL?

> +;; Because of these restrictions, this only expands on the desired cases.
> +(define_expand "feraiseexceptsi"
> +  [(use (match_operand:SI 1 "const_int_operand" "n"))
> +   (set (match_operand:SI 0 "gpc_reg_operand")
> +	(const_int 0))]
> +  "TARGET_HARD_FLOAT"
> +{
> +  switch (INTVAL (operands[1]))
> +    {
> +    case 0x2000000:  /* FE_INEXACT */
> +    case 0x4000000:  /* FE_DIVBYZERO */
> +    case 0x8000000:  /* FE_UNDERFLOW */
> +    case 0x10000000: /* FE_OVERFLOW */
> +      break;
> +    default:
> +      FAIL;
> +    }
> +
> +  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
> +  emit_insn (gen_rs6000_mtfsb1 (tmp));
> +  emit_move_insn (operands[0], const0_rtx);
> +  DONE;
> +})
> +

thwack extra line ? 

>  
>  ;; Define the TImode operations that can be done in a small number
>  ;; of instructions.  The & constraints are to prevent the register
> diff --git a/gcc/optabs.def b/gcc/optabs.def
> index 78409aa1453..987ee0f79dc 100644
> --- a/gcc/optabs.def
> +++ b/gcc/optabs.def
> @@ -318,6 +318,10 @@ OPTAB_D (sinh_optab, "sinh$a2")
>  OPTAB_D (tan_optab, "tan$a2")
>  OPTAB_D (tanh_optab, "tanh$a2")
> 
> +OPTAB_D (fegetround_optab, "fegetround$a")
> +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
> +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
> +
>  /* C99 implementations of fmax/fmin.  */
>  OPTAB_D (fmax_optab, "fmax$a3")
>  OPTAB_D (fmin_optab, "fmin$a3")
> diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
> new file mode 100644
> index 00000000000..a2977d188e5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-1.c
> @@ -0,0 +1,68 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target fenv_exceptions } */
> +/* { dg-options "-lm -fno-builtin" } */
> +
> +/* This testcase ensures that the builtins expand with the matching arguments
> + * or otherwise fallback gracefull to a function call, and don't ICE during
> + * compilation. */

gracefully

> +
> +#include <fenv.h>
> +
> +/* We use __builtin_* version to avoid the function be replaced by glibc that
> + * may have an inline optimization for it in fenv.h. */

s/to avoid .../to force the use of the gcc implementation of the
builtin, and avoid the glibc implementation./  ? 


No further comments or nits.. 
thanks
-WIll


> +int
> +main ()
> +{
> +  int   rsi = 0;
> +  long  rsl = 0;
> +  short rss = 0;
> +  char  rsc = 0;
> +
> +  unsigned int   rui = 0;
> +  unsigned long  rul = 0;
> +  unsigned short rus = 0;
> +  unsigned char  ruc = 0;
> +
> +  int e = FE_DIVBYZERO;
> +
> +  __builtin_feclearexcept(e);                          // CALL
> +  __builtin_feclearexcept(0);                          // CALL
> +  __builtin_feclearexcept(FE_ALL_EXCEPT);              // CALL
> +  __builtin_feclearexcept(FE_INVALID);                 // CALL
> +  __builtin_feclearexcept(FE_INEXACT | FE_DIVBYZERO);  // CALL
> +  __builtin_feclearexcept(FE_INEXACT);    // EXPAND
> +  __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  __builtin_feclearexcept(FE_UNDERFLOW);  // EXPAND
> +  __builtin_feclearexcept(FE_OVERFLOW);   // EXPAND
> +
> +  rsi = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  rsl = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  rss = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  rsc = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  rui = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  rul = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  rus = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +  ruc = __builtin_feclearexcept(FE_DIVBYZERO);  // EXPAND
> +
> +
> +  __builtin_feraiseexcept(e);                          // CALL
> +  __builtin_feraiseexcept(0);                          // CALL
> +  __builtin_feraiseexcept(FE_ALL_EXCEPT);              // CALL
> +  __builtin_feraiseexcept(FE_INVALID);                 // CALL
> +  __builtin_feraiseexcept(FE_INEXACT | FE_DIVBYZERO);  // CALL
> +  __builtin_feraiseexcept(FE_INEXACT);    // EXPAND
> +  __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  __builtin_feraiseexcept(FE_UNDERFLOW);  // EXPAND
> +  __builtin_feraiseexcept(FE_OVERFLOW);   // EXPAND
> +
> +  rsi = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  rsl = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  rss = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  rsc = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  rui = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  rul = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  rus = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +  ruc = __builtin_feraiseexcept(FE_DIVBYZERO);  // EXPAND
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
> new file mode 100644
> index 00000000000..4eb2a63d48f
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin-feclearexcept-feraiseexcept-2.c
> @@ -0,0 +1,133 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target fenv_exceptions } */
> +/* { dg-options "-lm -fno-builtin" } */
> +
> +/* This testcase ensures that the builtins are correctly expanded and match the
> + * expected result. */
> +
> +#include <fenv.h>
> +
> +#ifdef DEBUG
> +#include <stdio.h>
> +#define INFO(...) printf(__VA_ARGS__)
> +#define FAIL(v, e, x, s, f) \
> +        printf("ERROR [l %d] testing %s(%x): %s returned %x," \
> +               " expecected %x\n", __LINE__, s, x, f, v, e)
> +#else
> +void abort (void);
> +#define INFO(...)
> +#define FAIL(v, e, x, s, f) abort()
> +#endif
> +
> +/* We use __builtin_* version to avoid the function be replaced by glibc that
> + * may have an inline optimization for it in fenv.h. */
> +int
> +main ()
> +{
> +  char *s = 0;
> +  int e = 0;
> +  int raised = 0;
> +
> +  s = "FE_ALL_EXCEPT";
> +  e = FE_ALL_EXCEPT;
> +  INFO("test: %s(%x)\n", s, e);
> +
> +  feclearexcept(FE_ALL_EXCEPT);
> +  __builtin_feraiseexcept(FE_ALL_EXCEPT);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised != e)
> +    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
> +
> +  feraiseexcept(FE_ALL_EXCEPT);
> +  __builtin_feclearexcept(FE_ALL_EXCEPT);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised == FE_ALL_EXCEPT & ~e)
> +    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
> +
> +
> +  s = "FE_DIVBYZERO";
> +  e = FE_DIVBYZERO;
> +  INFO("test: %s(%x)\n", s, e);
> +
> +  feclearexcept(FE_ALL_EXCEPT);
> +  __builtin_feraiseexcept(FE_DIVBYZERO);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised != e)
> +    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
> +
> +  feraiseexcept(FE_ALL_EXCEPT);
> +  __builtin_feclearexcept(FE_DIVBYZERO);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised == FE_ALL_EXCEPT & ~e)
> +    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
> +
> +
> +  s = "FE_INEXACT";
> +  e = FE_INEXACT;
> +  INFO("test: %s(%x)\n", s, e);
> +
> +  feclearexcept(FE_ALL_EXCEPT);
> +  __builtin_feraiseexcept(FE_INEXACT);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised != e)
> +    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
> +
> +  feraiseexcept(FE_ALL_EXCEPT);
> +  __builtin_feclearexcept(FE_INEXACT);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised == FE_ALL_EXCEPT & ~e)
> +    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
> +
> +
> +  s = "FE_OVERFLOW";
> +  e = FE_OVERFLOW;
> +  INFO("test: %s(%x)\n", s, e);
> +
> +  feclearexcept(FE_ALL_EXCEPT);
> +  __builtin_feraiseexcept(FE_OVERFLOW);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised != e)
> +    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
> +
> +  feraiseexcept(FE_ALL_EXCEPT);
> +  __builtin_feclearexcept(FE_OVERFLOW);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised == FE_ALL_EXCEPT & ~e)
> +    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
> +
> +
> +  s = "FE_UNDERFLOW";
> +  e = FE_UNDERFLOW;
> +  INFO("test: %s(%x)\n", s, e);
> +
> +  feclearexcept(FE_ALL_EXCEPT);
> +  __builtin_feraiseexcept(FE_UNDERFLOW);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised != e)
> +    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
> +
> +  feraiseexcept(FE_ALL_EXCEPT);
> +  __builtin_feclearexcept(FE_UNDERFLOW);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised == FE_ALL_EXCEPT & ~e)
> +    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
> +
> +
> +  s = "FE_INVALID";
> +  e = FE_INVALID;
> +  INFO("test: %s(%x)\n", s, e);
> +
> +  feclearexcept(FE_ALL_EXCEPT);
> +  __builtin_feraiseexcept(FE_INVALID);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised != e)
> +    FAIL(raised, e, e, s, "__builtin_feraiseexcept");
> +
> +  feraiseexcept(FE_ALL_EXCEPT);
> +  __builtin_feclearexcept(FE_INVALID);
> +  raised = fetestexcept(FE_ALL_EXCEPT);
> +  if (raised == FE_ALL_EXCEPT & ~e)
> +    FAIL(raised, FE_ALL_EXCEPT & ~e, e, s, "__builtin_feclearexcept");
> +
> +  return 0;
> +}
> diff --git a/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c b/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c
> new file mode 100644
> index 00000000000..c3649f874a5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/builtin-fegetround.c
> @@ -0,0 +1,36 @@
> +/* { dg-do run } */
> +/* { dg-require-effective-target fenv_exceptions } */
> +/* { dg-options "-lm -fno-builtin" } */
> +
> +/* This testcase ensures that the builtins is correctly expanded and match the
> + * expected result from the standard function. */
> +
> +#include <fenv.h>
> +
> +#ifdef DEBUG
> +#include <stdio.h>
> +#define FAIL(v, e) printf("ERROR, __builtin_fegetround() returned %d," \
> +                          " not the expecected value %d\n", v, e);
> +#else
> +void abort (void);
> +#define FAIL(v, e) abort()
> +#endif
> +
> +/* We use __builtin_* version to avoid the function be replaced by glibc that
> + * may have an inline optimization for it in fenv.h. */
> +int
> +main ()
> +{
> +  int i, rounding, expected;
> +  const int rm[] = {FE_TONEAREST, FE_TOWARDZERO, FE_UPWARD, FE_DOWNWARD};
> +  for (i = 0; i < sizeof(rm); i++)
> +    {
> +      fesetround(rm[i]);
> +      rounding = __builtin_fegetround();
> +      expected = fegetround();
> +      if (rounding != expected)
> +        FAIL(rounding, expected);
> +    }
> +
> +  return 0;
> +}


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-04 15:52 [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193] Raoni Fassina Firmino
  2020-09-18 17:32 ` Raoni Fassina Firmino
  2020-09-28 16:42 ` will schmidt
@ 2020-09-29 23:38 ` Segher Boessenkool
  2020-09-30  7:02   ` Richard Biener
  2 siblings, 1 reply; 16+ messages in thread
From: Segher Boessenkool @ 2020-09-29 23:38 UTC (permalink / raw)
  To: gcc-patches, joseph; +Cc: Richard Biener, . Jakub Jelinek, Richard Sandiford

Hi Raoni,

Some of this isn't an rs6000 patch, but the subject says it is, so it
might well not draw the attention it needs.

Adding some Cc:s.

On Fri, Sep 04, 2020 at 12:52:30PM -0300, Raoni Fassina Firmino wrote:
> There is one pending question raised by Segher, It is about adding
> documentation, I am not sure if it is needed and if so, where it
> should be. I will quote the relevant part of the conversation[2] from
> the v1 thread for context:
> 
>   > > > +OPTAB_D (fegetround_optab, "fegetround$a")
>   > > > +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
>   > > > +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
>   > >␣
>   > > Should those be documented somewhere?  (In gcc/doc/ somewhere).
>   >
>   > I am lost on that one. I took a look on the docs (I hope looking on the
>   > online docs was good enough) and I didn't find a place where i feel it
>   > sits well. On the PowerPC target specific sections (6.60.22 Basic
>   > PowerPC Built-in Functions), I didn't found it mentioning builtins that
>   > are optimizations for the standard library functions, but we do have
>   > many of these for Power.  Then, on the generic section (6.59 Other
>   > Built-in Functions Provided by GCC) it mentions C99 functions that have
>   > builtins but it seems like it mentions builtins that have target
>   > independent implementation, or at least it dos not say that some
>   > builtins may be implemented on only some targets.  And in this case
>   > there is no implementation (for now) for any other target that is not
>   > PowerPc.
>   >
>   > So, I don't know if or where this should be documented.

I don't see much about optabs in the docs either.  Add some text to
optabs.def itself then?

> +(define_expand "feclearexceptsi"
> +  [(use (match_operand:SI 1 "const_int_operand" "n"))
> +   (set (match_operand:SI 0 "gpc_reg_operand")
> +	(const_int 0))]
> +  "TARGET_HARD_FLOAT"
> +{
> +  switch (INTVAL (operands[1]))
> +    {
> +    case 0x2000000:  /* FE_INEXACT */
> +    case 0x4000000:  /* FE_DIVBYZERO */
> +    case 0x8000000:  /* FE_UNDERFLOW */
> +    case 0x10000000: /* FE_OVERFLOW */

Please write 0x02000000 etc. instead?

> +;; int fegraiseexcept(int excepts)

(typo)

> +/* { dg-do run } */
> +/* { dg-require-effective-target fenv_exceptions } */
> +/* { dg-options "-lm -fno-builtin" } */

That -fno-builtin looks very strange...  Comment what it is for?

> +#define FAIL(v, e) printf("ERROR, __builtin_fegetround() returned %d," \
> +                          " not the expecected value %d\n", v, e);

(Typo, "expected")

The rs6000 part is okay for trunk (with those modifications), after the
generic parts is approved.  Thanks!


Segher

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-29 23:38 ` [PATCH v2] builtins: (not just) " Segher Boessenkool
@ 2020-09-30  7:02   ` Richard Biener
  2020-09-30 20:12     ` Segher Boessenkool
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Biener @ 2020-09-30  7:02 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: gcc-patches, joseph, . Jakub Jelinek, Richard Sandiford

On Tue, 29 Sep 2020, Segher Boessenkool wrote:

> Hi Raoni,
> 
> Some of this isn't an rs6000 patch, but the subject says it is, so it
> might well not draw the attention it needs.
> 
> Adding some Cc:s.
> 
> On Fri, Sep 04, 2020 at 12:52:30PM -0300, Raoni Fassina Firmino wrote:
> > There is one pending question raised by Segher, It is about adding
> > documentation, I am not sure if it is needed and if so, where it
> > should be. I will quote the relevant part of the conversation[2] from
> > the v1 thread for context:
> > 
> >   > > > +OPTAB_D (fegetround_optab, "fegetround$a")
> >   > > > +OPTAB_D (feclearexcept_optab, "feclearexcept$a")
> >   > > > +OPTAB_D (feraiseexcept_optab, "feraiseexcept$a")
> >   > >?
> >   > > Should those be documented somewhere?  (In gcc/doc/ somewhere).
> >   >
> >   > I am lost on that one. I took a look on the docs (I hope looking on the
> >   > online docs was good enough) and I didn't find a place where i feel it
> >   > sits well. On the PowerPC target specific sections (6.60.22 Basic
> >   > PowerPC Built-in Functions), I didn't found it mentioning builtins that
> >   > are optimizations for the standard library functions, but we do have
> >   > many of these for Power.  Then, on the generic section (6.59 Other
> >   > Built-in Functions Provided by GCC) it mentions C99 functions that have
> >   > builtins but it seems like it mentions builtins that have target
> >   > independent implementation, or at least it dos not say that some
> >   > builtins may be implemented on only some targets.  And in this case
> >   > there is no implementation (for now) for any other target that is not
> >   > PowerPc.
> >   >
> >   > So, I don't know if or where this should be documented.
> 
> I don't see much about optabs in the docs either.  Add some text to
> optabs.def itself then?

All optabs are documented in doc/md.texi as 'instruction patterns'

This is where new optabs need to be documented.

> > +(define_expand "feclearexceptsi"
> > +  [(use (match_operand:SI 1 "const_int_operand" "n"))
> > +   (set (match_operand:SI 0 "gpc_reg_operand")
> > +	(const_int 0))]
> > +  "TARGET_HARD_FLOAT"
> > +{
> > +  switch (INTVAL (operands[1]))
> > +    {
> > +    case 0x2000000:  /* FE_INEXACT */
> > +    case 0x4000000:  /* FE_DIVBYZERO */
> > +    case 0x8000000:  /* FE_UNDERFLOW */
> > +    case 0x10000000: /* FE_OVERFLOW */
> 
> Please write 0x02000000 etc. instead?
> 
> > +;; int fegraiseexcept(int excepts)
> 
> (typo)
> 
> > +/* { dg-do run } */
> > +/* { dg-require-effective-target fenv_exceptions } */
> > +/* { dg-options "-lm -fno-builtin" } */
> 
> That -fno-builtin looks very strange...  Comment what it is for?
> 
> > +#define FAIL(v, e) printf("ERROR, __builtin_fegetround() returned %d," \
> > +                          " not the expecected value %d\n", v, e);
> 
> (Typo, "expected")
> 
> The rs6000 part is okay for trunk (with those modifications), after the
> generic parts is approved.  Thanks!
> 
> 
> Segher
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-30  7:02   ` Richard Biener
@ 2020-09-30 20:12     ` Segher Boessenkool
  2020-10-01  6:08       ` Richard Biener
  0 siblings, 1 reply; 16+ messages in thread
From: Segher Boessenkool @ 2020-09-30 20:12 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, joseph, . Jakub Jelinek, Richard Sandiford

On Wed, Sep 30, 2020 at 09:02:34AM +0200, Richard Biener wrote:
> On Tue, 29 Sep 2020, Segher Boessenkool wrote:
> > I don't see much about optabs in the docs either.  Add some text to
> > optabs.def itself then?
> 
> All optabs are documented in doc/md.texi as 'instruction patterns'

Except for what seems to be the majority that isn't.

> This is where new optabs need to be documented.

It's going to be challenging to find a reasonable spot in there.
Oh well.

Thanks,


Segher

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-30 20:12     ` Segher Boessenkool
@ 2020-10-01  6:08       ` Richard Biener
  2020-10-01 20:08         ` Segher Boessenkool
  0 siblings, 1 reply; 16+ messages in thread
From: Richard Biener @ 2020-10-01  6:08 UTC (permalink / raw)
  To: Segher Boessenkool
  Cc: gcc-patches, joseph, . Jakub Jelinek, Richard Sandiford

On Wed, 30 Sep 2020, Segher Boessenkool wrote:

> On Wed, Sep 30, 2020 at 09:02:34AM +0200, Richard Biener wrote:
> > On Tue, 29 Sep 2020, Segher Boessenkool wrote:
> > > I don't see much about optabs in the docs either.  Add some text to
> > > optabs.def itself then?
> > 
> > All optabs are documented in doc/md.texi as 'instruction patterns'
> 
> Except for what seems to be the majority that isn't.

Really?  Everytime I looked for one I found it.

> > This is where new optabs need to be documented.
> 
> It's going to be challenging to find a reasonable spot in there.
> Oh well.

Put it next to fmin/fmax docs or sin, etc. - at least the section
should be clear ;)  But yeah, patterns seem to be quite randomly
"sorted"...

Richard.

> Thanks,
> 
> 
> Segher
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-10-01  6:08       ` Richard Biener
@ 2020-10-01 20:08         ` Segher Boessenkool
  2020-10-26 15:19           ` Raoni Fassina Firmino
  0 siblings, 1 reply; 16+ messages in thread
From: Segher Boessenkool @ 2020-10-01 20:08 UTC (permalink / raw)
  To: Richard Biener; +Cc: gcc-patches, joseph, . Jakub Jelinek, Richard Sandiford

On Thu, Oct 01, 2020 at 08:08:01AM +0200, Richard Biener wrote:
> On Wed, 30 Sep 2020, Segher Boessenkool wrote:
> 
> > On Wed, Sep 30, 2020 at 09:02:34AM +0200, Richard Biener wrote:
> > > On Tue, 29 Sep 2020, Segher Boessenkool wrote:
> > > > I don't see much about optabs in the docs either.  Add some text to
> > > > optabs.def itself then?
> > > 
> > > All optabs are documented in doc/md.texi as 'instruction patterns'
> > 
> > Except for what seems to be the majority that isn't.
> 
> Really?  Everytime I looked for one I found it.

Yeah, all the obvious ones are documented of course.  I only looked at
the non-obvious ones now, to see where these fe* should go, and maybe I
just had some unlucky picks, but many of those are not documented it
seems.  "The majority are not" is a gross exaggeration ;-)

> > > This is where new optabs need to be documented.
> > 
> > It's going to be challenging to find a reasonable spot in there.
> > Oh well.
> 
> Put it next to fmin/fmax docs or sin, etc. - at least the section
> should be clear ;)  But yeah, patterns seem to be quite randomly
> "sorted"...

And no chapter toc etc.

Yeah, this should just go with the other fp things, of course.  Duh.
Thanks!


Segher

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-28 16:42 ` will schmidt
@ 2020-10-05  1:56   ` Hans-Peter Nilsson
  2020-10-05 15:36     ` Segher Boessenkool
  2020-10-27 19:25     ` Segher Boessenkool
  2020-10-26 16:05   ` Raoni Fassina Firmino
  1 sibling, 2 replies; 16+ messages in thread
From: Hans-Peter Nilsson @ 2020-10-05  1:56 UTC (permalink / raw)
  To: will schmidt; +Cc: Raoni Fassina Firmino, gcc-patches, segher, joseph

Please excuse a comment from the gallery:

On Mon, 28 Sep 2020, will schmidt via Gcc-patches wrote:
> On Fri, 2020-09-04 at 12:52 -0300, Raoni Fassina Firmino via Gcc-patches wrote:

> > 2020-08-13  Raoni Fassina Firmino  <raoni@linux.ibm.com>
> >
> > gcc/ChangeLog:

> > 	* config/rs6000/rs6000.md (fegetroundsi): New pattern.
> > 	(feclearexceptsi): New Pattern.
> > 	(feraiseexceptsi): New Pattern.

> > +(define_expand "feraiseexceptsi"
> > +  [(use (match_operand:SI 1 "const_int_operand" "n"))
> > +   (set (match_operand:SI 0 "gpc_reg_operand")
> > +	(const_int 0))]
> > +  "TARGET_HARD_FLOAT"
> > +{
> > +  switch (INTVAL (operands[1]))
> > +    {
> > +    case 0x2000000:  /* FE_INEXACT */
> > +    case 0x4000000:  /* FE_DIVBYZERO */
> > +    case 0x8000000:  /* FE_UNDERFLOW */
> > +    case 0x10000000: /* FE_OVERFLOW */
> > +      break;
> > +    default:
> > +      FAIL;
> > +    }
> > +
> > +  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));

This doesn't appear to be very portable, to any-cxx11-compiler
that doesn't pretend to be gcc-intrinsics-compatible.

If the valid values had been more complicated, there'd be
bitmap.c:bitmap_last_set_bit to follow for a suitable portable
pattern.  It conditionalizes like:
#if GCC_VERSION >= 3004
  gcc_assert (sizeof (long) == sizeof (word));
  bit_no += BITMAP_WORD_BITS - __builtin_clzl (word) - 1;
#else
...
#endif

Better, there's "ffs".

brgds, H-P
PS. No less than four targets fail like that.  Meh.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-10-05  1:56   ` Hans-Peter Nilsson
@ 2020-10-05 15:36     ` Segher Boessenkool
  2020-10-26 15:23       ` Raoni Fassina Firmino
  2020-10-27 19:25     ` Segher Boessenkool
  1 sibling, 1 reply; 16+ messages in thread
From: Segher Boessenkool @ 2020-10-05 15:36 UTC (permalink / raw)
  To: Hans-Peter Nilsson
  Cc: will schmidt, Raoni Fassina Firmino, gcc-patches, joseph

Hi!

On Sun, Oct 04, 2020 at 09:56:01PM -0400, Hans-Peter Nilsson wrote:
> Please excuse a comment from the gallery:
> 
> On Mon, 28 Sep 2020, will schmidt via Gcc-patches wrote:
> > On Fri, 2020-09-04 at 12:52 -0300, Raoni Fassina Firmino via Gcc-patches wrote:
> > > +(define_expand "feraiseexceptsi"
> > > +  [(use (match_operand:SI 1 "const_int_operand" "n"))
> > > +   (set (match_operand:SI 0 "gpc_reg_operand")
> > > +	(const_int 0))]
> > > +  "TARGET_HARD_FLOAT"
> > > +{
> > > +  switch (INTVAL (operands[1]))
> > > +    {
> > > +    case 0x2000000:  /* FE_INEXACT */
> > > +    case 0x4000000:  /* FE_DIVBYZERO */
> > > +    case 0x8000000:  /* FE_UNDERFLOW */
> > > +    case 0x10000000: /* FE_OVERFLOW */
> > > +      break;
> > > +    default:
> > > +      FAIL;
> > > +    }
> > > +
> > > +  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
> 
> This doesn't appear to be very portable, to any-cxx11-compiler
> that doesn't pretend to be gcc-intrinsics-compatible.

Yeah, very good point!

Should this pattern not allow setting more than one exception bit at
once, btw?

So you can first see if any out-of-range bits are set:

  unsigned int fe = INTVAL (operands[1]);
  if ((fe & 0x1e000000) != fe)
    FAIL;

and then see if more than one bit is set:

  if (fe & (fe - 1))
    FAIL;

but also disallow zero:

  if (!fe)
    FAIL;

Or, you can just put the bit number in the switch cases for the four
bits.  There are only four, after all.

Thanks,


Segher

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: (not just) rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-10-01 20:08         ` Segher Boessenkool
@ 2020-10-26 15:19           ` Raoni Fassina Firmino
  0 siblings, 0 replies; 16+ messages in thread
From: Raoni Fassina Firmino @ 2020-10-26 15:19 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Richard Biener, . Jakub Jelinek, gcc-patches, joseph

On Thu, Oct 01, 2020 at 03:08:19PM -0500, Segher Boessenkool wrote:
> On Thu, Oct 01, 2020 at 08:08:01AM +0200, Richard Biener wrote:
> > On Wed, 30 Sep 2020, Segher Boessenkool wrote:
> > > It's going to be challenging to find a reasonable spot in there.
> > > Oh well.
> > 
> > Put it next to fmin/fmax docs or sin, etc. - at least the section
> > should be clear ;)  But yeah, patterns seem to be quite randomly
> > "sorted"...
> 
> And no chapter toc etc.
> 
> Yeah, this should just go with the other fp things, of course.  Duh.
> Thanks!

I doesn't help that the order in doc/md.texi is different than in
optabs.def.

I ended up putting it after all the trigonometric ones.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-10-05 15:36     ` Segher Boessenkool
@ 2020-10-26 15:23       ` Raoni Fassina Firmino
  0 siblings, 0 replies; 16+ messages in thread
From: Raoni Fassina Firmino @ 2020-10-26 15:23 UTC (permalink / raw)
  To: Segher Boessenkool; +Cc: Hans-Peter Nilsson, will schmidt, gcc-patches, joseph

On Mon, Oct 05, 2020 at 10:36:22AM -0500, Segher Boessenkool wrote:
> Should this pattern not allow setting more than one exception bit at
> once, btw?

Turns out allowing more than one bit was no problem at all.

On Mon, Oct 05, 2020 at 10:36:22AM -0500, Segher Boessenkool wrote:
> On Sun, Oct 04, 2020 at 09:56:01PM -0400, Hans-Peter Nilsson wrote:
> > > > +  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
> > 
> > This doesn't appear to be very portable, to any-cxx11-compiler
> > that doesn't pretend to be gcc-intrinsics-compatible.
> 
> Yeah, very good point!

And with that, no ffs() or clz() necessary at all :)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-09-28 16:42 ` will schmidt
  2020-10-05  1:56   ` Hans-Peter Nilsson
@ 2020-10-26 16:05   ` Raoni Fassina Firmino
  2020-10-26 16:54     ` Segher Boessenkool
  1 sibling, 1 reply; 16+ messages in thread
From: Raoni Fassina Firmino @ 2020-10-26 16:05 UTC (permalink / raw)
  To: will schmidt; +Cc: gcc-patches, joseph, segher

On Mon, Sep 28, 2020 at 11:42:13AM -0500, will schmidt wrote:
> > +/* Expand call EXP to either feclearexcept or feraiseexcept builtins (from C99
> > +    fenv.h), returning the result and setting it in TARGET.  Otherwise return
> > +    NULL_RTX on failure.  */
> > +static rtx
> > +expand_builtin_feclear_feraise_except (tree exp, rtx target,
> > +				       machine_mode target_mode, optab op_optab)
> > +{
> > +  if (!validate_arglist (exp, INTEGER_TYPE, VOID_TYPE))
> > +    return NULL_RTX;
> > +  rtx op0 = expand_normal (CALL_EXPR_ARG (exp, 0));
> > +
> > +  insn_code icode = direct_optab_handler (op_optab, SImode);
> > +  if (icode == CODE_FOR_nothing)
> > +    return NULL_RTX;
> > +
> > +  if (target == 0
> > +      || GET_MODE (target) != target_mode
> > +      || ! (*insn_data[icode].operand[0].predicate) (target, target_mode))
> > +    target = gen_reg_rtx (target_mode);
> > +
> > +  rtx pat = GEN_FCN (icode) (target, op0);
> > +  if (!pat)
> > +    return NULL_RTX;
> > +  emit_insn (pat);
> > +
> > +  return target;
> > +}
> 
> 
> I don't see any references to feclearexcept or feraiseexcept in the
> functions there.   I see the callers pass in those values via optab,
> but nothing in these functions explicitly checks or limits that in a
> way that is obvious upon my reading...  Thus I wonder if there may be a
> different generic names that would be appropriate for the functions.

Yes, I wonder the same, I guess in theory it could be used with any
int(int) builtin at least, but looking at other more generic
expand_builtin_* I was not confident enough that this one has the
necessary boilerplate code to handle other builtins.  Also I looked
aroung builtins.c trying to find an expand that I could reuse and I
notice that the vast majority of builtins use dedicated expand_* so I
just followed suit.  I am not really trilled by this name anyway, so If
it something useful in a more generic way (which I don't have enough
knowledge to judge) I am happy to change it.


> > +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> > +;; It doesn't handle values out of range, and always returns 0.
> > +;; Note that FE_INVALID is unsupported because it maps to more than
> > +;; one bit on FPSCR register.
> 
> Should FE_INVALID have an explicit case statement path to FAIL?

Because there is only 4 valid flags I am doing the other way around,
just checking if it is any of the valid flags and FAIL for any other
value, so there is no need of an explicit FE_INVALID case, but if it is
better to have one anyway to make the intention clear through code, I
don't know.


> No further comments or nits..

I applied all other suggestions for now,
thanks for the feedback Will :)


o/
Raoni

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-10-26 16:05   ` Raoni Fassina Firmino
@ 2020-10-26 16:54     ` Segher Boessenkool
  0 siblings, 0 replies; 16+ messages in thread
From: Segher Boessenkool @ 2020-10-26 16:54 UTC (permalink / raw)
  To: will schmidt, gcc-patches, joseph

On Mon, Oct 26, 2020 at 01:05:00PM -0300, Raoni Fassina Firmino wrote:
> On Mon, Sep 28, 2020 at 11:42:13AM -0500, will schmidt wrote:
> > > +;; FE_INEXACT, FE_DIVBYZERO, FE_UNDERFLOW and FE_OVERFLOW flags.
> > > +;; It doesn't handle values out of range, and always returns 0.
> > > +;; Note that FE_INVALID is unsupported because it maps to more than
> > > +;; one bit on FPSCR register.
> > 
> > Should FE_INVALID have an explicit case statement path to FAIL?
> 
> Because there is only 4 valid flags I am doing the other way around,
> just checking if it is any of the valid flags and FAIL for any other
> value, so there is no need of an explicit FE_INVALID case, but if it is
> better to have one anyway to make the intention clear through code, I
> don't know.

To clear VX ("invalid", bit 34) you need to clear all different VX bits
(one per cause), so 39-44, 53-55.  To *set* it you need to pick which
one you want to set.  Maybe this is all best left the the libc in use,
which should have its own policy for that, it might not be the same on
all libcs.


Segher

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193]
  2020-10-05  1:56   ` Hans-Peter Nilsson
  2020-10-05 15:36     ` Segher Boessenkool
@ 2020-10-27 19:25     ` Segher Boessenkool
  1 sibling, 0 replies; 16+ messages in thread
From: Segher Boessenkool @ 2020-10-27 19:25 UTC (permalink / raw)
  To: Hans-Peter Nilsson
  Cc: will schmidt, Raoni Fassina Firmino, gcc-patches, joseph

Hi!

On Sun, Oct 04, 2020 at 09:56:01PM -0400, Hans-Peter Nilsson wrote:
> Please excuse a comment from the gallery:

:-)  Thanks!

> > > +  rtx tmp = gen_rtx_CONST_INT (SImode, __builtin_clz (INTVAL (operands[1])));
> 
> This doesn't appear to be very portable, to any-cxx11-compiler
> that doesn't pretend to be gcc-intrinsics-compatible.

Yup, thanks for spotting this :-)

> PS. No less than four targets fail like that.  Meh.

$ grep -r __builtin_clz config/
config/tilepro/gen-mul-tables.cc:  int prev_pow2 = 1 << (31 - __builtin_clz (abs_multiplier));
config/tilepro/gen-mul-tables.cc:      1LL << (63 - __builtin_clzll (abs_multiplier));

This is a generator program that is not run on normal builds, so
arguably not a bug.

config/nds32/nds32-md-auxiliary.c:  clear_sign_bit_copies =  __builtin_clz (remainder);
config/arc/arc.c:           if ((GMASK_LEN - __builtin_clzll (gmask)) == (i + 1)
config/arc/arc.c:           if ((GMASK_LEN - __builtin_clzll (gmask)) == i

Those seem bugs yes.

config/rs6000/bmi2intrin.h:      c = __builtin_clzl (m);
config/rs6000/bmi2intrin.h:       c = __builtin_clzl (m);
config/rs6000/bmi2intrin.h:       c = __builtin_clzl (m);
config/rs6000/ppu_intrinsics.h:#define __cntlzw(v) __builtin_clz(v)
config/rs6000/ppu_intrinsics.h:#define __cntlzd(v) __builtin_clzll(v)

These are installed headers, only used with the built GCC.  No bug.

config/i386/i386-builtin.def:BDESC (OPTION_MASK_ISA_LZCNT, 0, CODE_FOR_lzcnt_hi, "__builtin_clzs", IX86_BUILTIN_CLZS, UNKNOWN, (int) UINT16_FTYPE_UINT16)

A different thing altogether, my grep was a bit wide :-)  But not a bug
anyway.

config/aarch64/aarch64.c:  return val == mask * bitmask_imm_mul[__builtin_clz (bits) - 26];

This seems bad as well, yes.

Thanks,


Segher

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-10-27 19:26 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-04 15:52 [PATCH v2] builtins: rs6000: Add builtins for fegetround, feclearexcept and feraiseexcept [PR94193] Raoni Fassina Firmino
2020-09-18 17:32 ` Raoni Fassina Firmino
2020-09-28 12:41   ` Raoni Fassina Firmino
2020-09-28 16:42 ` will schmidt
2020-10-05  1:56   ` Hans-Peter Nilsson
2020-10-05 15:36     ` Segher Boessenkool
2020-10-26 15:23       ` Raoni Fassina Firmino
2020-10-27 19:25     ` Segher Boessenkool
2020-10-26 16:05   ` Raoni Fassina Firmino
2020-10-26 16:54     ` Segher Boessenkool
2020-09-29 23:38 ` [PATCH v2] builtins: (not just) " Segher Boessenkool
2020-09-30  7:02   ` Richard Biener
2020-09-30 20:12     ` Segher Boessenkool
2020-10-01  6:08       ` Richard Biener
2020-10-01 20:08         ` Segher Boessenkool
2020-10-26 15:19           ` Raoni Fassina Firmino

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).