Patchwork RX cleanup rx_option_optimization

login
register
mail settings
Submitter Joseph S. Myers
Date Oct. 15, 2010, 12:40 a.m.
Message ID <Pine.LNX.4.64.1010150037540.2442@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/67877/
State New
Headers show

Comments

Joseph S. Myers - Oct. 15, 2010, 12:40 a.m.
The RX GCC port has a function rx_option_optimization that appears
rather confused.

This function sets -ffast-math if TARGET_USE_FPU when it is first
called.  It is first called before any target options have been
processed, so this effectively sets -ffast-math unconditionally,
independent of whether the FPU would end up being used after all the
options have been processed.

The function disables LTO compression.  This is the sort of hack that
shouldn't have got into FSF sources at all; whatever problems someone
might have had with LTO compression, they are surely not
target-specific in any way and so should not go in the sources of a
target port.

The function checks for changes to whether FPU instructions may be
used.  But such changes aren't possible since this port doesn't
support the "target" attribute and the "optimize" attribute doesn't
allow changing target options.

The function gives warnings if options are changed to disable
fast-math when allowing FPU instructions.  But this isn't the right
place for such warnings since this function will be called too early
in the processing of an optimize attribute for any -f options to have
taken effect.

What this function does makes much more sense if it is a
TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE hook rather than
TARGET_OPTION_OPTIMIZATION, so this patch adjusts it accordingly, as
well as removing the flag_lto_compression_level changes and the checks
for changes to whether the FPU can be used.  The result isn't perfect,
in that it should take account of -fno-fast-math before implying
-ffast-math, but it's closer to what I think was intended.

Tested by building cc1 for a cross to rx-elf.  OK to commit?

In general this setting -ffast-math seems suspicious.  What exactly
are the non-IEEE properties of the RX FPU?  Is it really the case that
when results are normal, finite values, the FPU will still return
inaccurate results?  If not, then only the relevant flags should be
set, not the whole of -ffast-math.  For example, if it doesn't support
NaNs and infinities, set flag_finite_math_only; if it doesn't support
signed zeros, clear flag_signed_zeros; if it doesn't support
exceptions, clear flag_trapping_math; if it doesn't support rounding
modes, clear flag_rounding_math; if it always uses a rounding mode
other than round-to-nearest, use a variant floating-point format like
SPU does.  Other back ends set relevant flags, not the whole of
-ffast-math.

2010-10-14  Joseph Myers  <joseph@codesourcery.com>

	* config/rx/rx.c (rx_option_optimization): Change to
	rx_override_options_after_change.  Don't change
	flag_lto_compression_level.  Don't check for changes to whether
	FPU instructions can be used.
	(rx_option_override): Call rx_override_options_after_change.
	(TARGET_OPTION_OPTIMIZATION): Remove.
	(TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): Define.
Nick Clifton - Oct. 15, 2010, 8:13 a.m.
Hi Joseph,

> In general this setting -ffast-math seems suspicious.  What exactly
> are the non-IEEE properties of the RX FPU?  Is it really the case that
> when results are normal, finite values, the FPU will still return
> inaccurate results?

No.

 > If not, then only the relevant flags should be
> set, not the whole of -ffast-math.  For example, if it doesn't support
> NaNs and infinities, set flag_finite_math_only;

This is what should be happening.  If you would care to make that change 
to your patch then that is pre-approved.

> 2010-10-14  Joseph Myers<joseph@codesourcery.com>
>
> 	* config/rx/rx.c (rx_option_optimization): Change to
> 	rx_override_options_after_change.  Don't change
> 	flag_lto_compression_level.  Don't check for changes to whether
> 	FPU instructions can be used.
> 	(rx_option_override): Call rx_override_options_after_change.
> 	(TARGET_OPTION_OPTIMIZATION): Remove.
> 	(TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE): Define.

Approved.

Cheers
   Nick

Patch

Index: gcc/config/rx/rx.c
===================================================================
--- gcc/config/rx/rx.c	(revision 165460)
+++ gcc/config/rx/rx.c	(working copy)
@@ -2191,13 +2191,12 @@  rx_handle_option (size_t code, const cha
   return true;
 }
 
-/* Implement TARGET_OPTION_OPTIMIZATION.  */
+/* Implement TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE.  */
 
 static void
-rx_option_optimization (int level ATTRIBUTE_UNUSED, int size ATTRIBUTE_UNUSED)
+rx_override_options_after_change (void)
 {
   static bool first_time = TRUE;
-  static bool saved_allow_rx_fpu = TRUE;
 
   if (first_time)
     {
@@ -2207,13 +2206,6 @@  rx_option_optimization (int level ATTRIB
       if (TARGET_USE_FPU)
 	set_fast_math_flags (true);
 
-      /* FIXME: For some unknown reason LTO compression is not working,
-	 at least on my local system.  So set the default compression
-	 level to none, for now.  */
-      if (flag_lto_compression_level == -1)
-        flag_lto_compression_level = 0;
-
-      saved_allow_rx_fpu = ALLOW_RX_FPU_INSNS;
       first_time = FALSE;
     }
   else
@@ -2223,9 +2215,6 @@  rx_option_optimization (int level ATTRIB
       if (TARGET_USE_FPU
 	  && ! fast_math_flags_set_p ())
 	warning (0, "RX FPU instructions are not IEEE compliant");
-
-      if (saved_allow_rx_fpu != ALLOW_RX_FPU_INSNS)
-	error ("Changing the FPU insns/math optimizations pairing is not supported");
     }
 }
 
@@ -2235,6 +2224,8 @@  rx_option_override (void)
   /* This target defaults to strict volatile bitfields.  */
   if (flag_strict_volatile_bitfields < 0)
     flag_strict_volatile_bitfields = 1;
+
+  rx_override_options_after_change ();
 }
 
 
@@ -2834,8 +2825,8 @@  rx_memory_move_cost (enum machine_mode m
 #undef  TARGET_OPTION_OVERRIDE
 #define TARGET_OPTION_OVERRIDE			rx_option_override
 
-#undef  TARGET_OPTION_OPTIMIZATION
-#define TARGET_OPTION_OPTIMIZATION		rx_option_optimization
+#undef  TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE
+#define TARGET_OVERRIDE_OPTIONS_AFTER_CHANGE	rx_override_options_after_change
 
 #undef  TARGET_EXCEPT_UNWIND_INFO
 #define TARGET_EXCEPT_UNWIND_INFO		sjlj_except_unwind_info