Committed, MMIX: don't expand __builtin_ffs to ffs
diff mbox series

Message ID alpine.BSF.2.20.16.1809172243500.31651@arjuna.pair.com
State New
Headers show
Series
  • Committed, MMIX: don't expand __builtin_ffs to ffs
Related show

Commit Message

Hans-Peter Nilsson Sept. 18, 2018, 2:48 a.m. UTC
I tried to update newlib (from an old copy from couple of years ago),
but got curious regressions localized to tests related to ffs
handling:


brgds, H-P

Patch
diff mbox series

--- regress.prev        2018-09-13 18:49:04.130070398 +0200
+++ regress     2018-09-13 22:59:25.551637529 +0200
@@ -0,0 +1,5 @@ 
+gcc.sum gcc.c-torture/execute/builtin-bitops-1.c
+gcc.sum gcc.c-torture/execute/ffs-1.c
+gcc.sum gcc.c-torture/execute/ffs-2.c
+gcc.sum gcc.c-torture/execute/pr61725.c
+gcc.sum gcc.dg/pr85376.c

The added comment above mmix_init_libfuncs says the most, but
see also the special case in optab-libfuncs.c:init_optabs.
There's no __ffssi2 in libgcc for "64-bit targets" (__ffsSI2
becomes __ffsdi2).  This may not be worthwile of more elegant
generic handling, but to do this properly, the middle-end would
have to open-code promotion of arguments and call __ffsdi2 for
__builtin_ffs instead of the cop-out of forwarding to ffs, which
is bound to break similarly for other targets.  I guess no other
target has seen this only because LP64 targets with neither ffs
nor leading nor trailing zeros instructions are rare.  I intend
to back-port this to the gcc-8-branch too, given the usual
freedom for non-primary-or-secondary targets (not strictly a
regression).

The mentioned regressions were fixed with this; committed.

gcc:
	Handle a library implementation of ffs calling __builtin_ffs.
	* config/mmix/mmix.c (TARGET_INIT_LIBFUNCS): Override with...
	(mmix_init_libfuncs): New function: make __builtin_ffs expand
	to __ffsdi2.

Index: gcc/config/mmix/mmix.c
===================================================================
--- gcc/config/mmix/mmix.c	(revision 264350)
+++ gcc/config/mmix/mmix.c	(working copy)
@@ -33,6 +33,7 @@  along with GCC; see the file COPYING3.
 #include "memmodel.h"
 #include "tm_p.h"
 #include "insn-config.h"
+#include "optabs.h"
 #include "regs.h"
 #include "emit-rtl.h"
 #include "recog.h"
@@ -140,6 +141,7 @@  static void mmix_setup_incoming_varargs
   (cumulative_args_t, machine_mode, tree, int *, int);
 static void mmix_file_start (void);
 static void mmix_file_end (void);
+static void mmix_init_libfuncs (void);
 static bool mmix_rtx_costs (rtx, machine_mode, int, int, int *, bool);
 static int mmix_register_move_cost (machine_mode,
 				    reg_class_t, reg_class_t);
@@ -221,6 +223,9 @@  static HOST_WIDE_INT mmix_starting_frame
 #undef TARGET_ASM_OUTPUT_SOURCE_FILENAME
 #define TARGET_ASM_OUTPUT_SOURCE_FILENAME mmix_asm_output_source_filename

+#undef TARGET_INIT_LIBFUNCS
+#define TARGET_INIT_LIBFUNCS mmix_init_libfuncs
+
 #undef TARGET_CONDITIONAL_REGISTER_USAGE
 #define TARGET_CONDITIONAL_REGISTER_USAGE mmix_conditional_register_usage

@@ -1308,6 +1313,20 @@  mmix_asm_output_source_filename (FILE *s
   fprintf (stream, "\n");
 }

+/* Unfortunately, by default __builtin_ffs is expanded to ffs for
+   targets where INT_TYPE_SIZE < BITS_PER_WORD.  That together with
+   newlib since 2017-07-04 implementing ffs as __builtin_ffs leads to
+   (newlib) ffs recursively calling itself.  But, because of argument
+   promotion, and with ffs we're counting from the least bit, the
+   libgcc equivalent for ffsl works equally well for int arguments, so
+   just use that.  */
+
+static void
+mmix_init_libfuncs (void)
+{
+  set_optab_libfunc (ffs_optab, SImode, "__ffsdi2");
+}
+
 /* OUTPUT_QUOTED_STRING.  */

 void