PR33713: remove -fforce-addr

PR33713: remove -fforce-addr

[Aldy Hernandez]

Hi folks.

I'm picking up this patch that Steven attached to the PR but didn't
submit to the list.  The patch removes support for -fforce-addr.

	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33713

I have added a changelog, incorporated the change suggested by Paolo,
fixed a typo, and tested it on x86_64-linux.  There are no regressions.

Is this OK for mainline?

	PR33713
	* doc/invoke.texi: Remove -fforce-addr documentation.
	* expr.c (emit_move_insn): Remove use of flag_force_addr.
	(expand_expr_real_1): Same.
	(do_tablejump): Same.
	Call memory_address instead of memory_address_noforce.
	* expr.h (memory_address_noforce): Remove prototype.
	* explow.c (memory_address): Remove support for flag_force_addr.
	(validize_mem): Same.
	(memory_address_noforce): Remove.
	* common.opt: Add dummy documentation for -fforce-addr.
	* combine.c (can_combine_p): Remove -fforce-addr comment.
	* config/cris/cris.h: Update CC1_SPEC comment with regards to
	-fforce-addr.
	(OPTIMIZATION_OPTIONS): Remove set of flag_force_addr.
	* config/m68k/m68k.h (PIC_CASE_VECTOR_ADDRESS): Remove comment
	relating to memory_address_noforce.

	* gcc.c-torture/compile/20050802-1.c: Remove.
	* gcc.c-torture/compile/20011113-1.c: Remove.

Index: doc/invoke.texi
===================================================================
--- doc/invoke.texi	(revision 130511)
+++ doc/invoke.texi	(working copy)
@@ -324,7 +324,7 @@ Objective-C and Objective-C++ Dialects}.
 -fcse-skip-blocks -fcx-limited-range -fdata-sections -fdce -fdce @gol
 -fdelayed-branch -fdelete-null-pointer-checks -fdse -fdse @gol
 -fearly-inlining -fexpensive-optimizations -ffast-math @gol
--ffinite-math-only -ffloat-store -fforce-addr -fforward-propagate @gol
+-ffinite-math-only -ffloat-store -fforward-propagate @gol
 -ffunction-sections -fgcse -fgcse-after-reload -fgcse-las -fgcse-lm @gol
 -fgcse-sm -fif-conversion -fif-conversion2 -finline-functions @gol
 -finline-functions-called-once -finline-limit=@var{n} @gol
@@ -5140,11 +5140,6 @@ function calls and pops them all at once
 
 Disabled at levels @option{-O}, @option{-O2}, @option{-O3}, @option{-Os}.
 
-@item -fforce-addr
-@opindex fforce-addr
-Force memory address constants to be copied into registers before
-doing arithmetic on them.
-
 @item -fforward-propagate
 @opindex fforward-propagate
 Perform a forward propagation pass on RTL.  The pass tries to combine two
Index: testsuite/gcc.c-torture/compile/20050802-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/20050802-1.c	(revision 130511)
+++ testsuite/gcc.c-torture/compile/20050802-1.c	(working copy)
@@ -1,10 +0,0 @@
-/* PR 23196 */
-/* { dg-options "-fforce-addr" } */
-
-void foo()
-{
-  char c;
-
-  c |= 1;
-  bar(&c);
-}
Index: testsuite/gcc.dg/20011113-1.c
===================================================================
--- testsuite/gcc.dg/20011113-1.c	(revision 130511)
+++ testsuite/gcc.dg/20011113-1.c	(working copy)
@@ -1,16 +0,0 @@
-/* Copyright (C) 2001  Free Software Foundation.
-   by Hans-Peter Nilsson  <hp@axis.com>  */
-
-/* { dg-do compile } */
-/* { dg-options "-O2 -fforce-addr" } */
-
-const char foo[] = "fum";
-const struct fi
-{
-  const char *const in;
-  const char *const out;
-  const int flags;
-} fie[] = {
-  {"nw", " new", 0},
-  {"dl", foo, 1}
-};
Index: expr.c
===================================================================
--- expr.c	(revision 130511)
+++ expr.c	(working copy)
@@ -3391,16 +3391,12 @@ emit_move_insn (rtx x, rtx y)
   /* If X or Y are memory references, verify that their addresses are valid
      for the machine.  */
   if (MEM_P (x)
-      && ((! memory_address_p (GET_MODE (x), XEXP (x, 0))
-	   && ! push_operand (x, GET_MODE (x)))
-	  || (flag_force_addr
-	      && CONSTANT_ADDRESS_P (XEXP (x, 0)))))
+      && (! memory_address_p (GET_MODE (x), XEXP (x, 0))
+	  && ! push_operand (x, GET_MODE (x))))
     x = validize_mem (x);
 
   if (MEM_P (y)
-      && (! memory_address_p (GET_MODE (y), XEXP (y, 0))
-	  || (flag_force_addr
-	      && CONSTANT_ADDRESS_P (XEXP (y, 0)))))
+      && ! memory_address_p (GET_MODE (y), XEXP (y, 0)))
     y = validize_mem (y);
 
   gcc_assert (mode != BLKmode);
@@ -7294,9 +7290,8 @@ expand_expr_real_1 (tree exp, rtx target
       if (MEM_P (decl_rtl) && REG_P (XEXP (decl_rtl, 0)))
 	temp = validize_mem (decl_rtl);
 
-      /* If DECL_RTL is memory, we are in the normal case and either
-	 the address is not valid or it is not a register and -fforce-addr
-	 is specified, get the address into a register.  */
+      /* If DECL_RTL is memory, we are in the normal case and the
+	 address is not valid, get the address into a register.  */
 
       else if (MEM_P (decl_rtl) && modifier != EXPAND_INITIALIZER)
 	{
@@ -7305,8 +7300,7 @@ expand_expr_real_1 (tree exp, rtx target
 	  decl_rtl = use_anchored_address (decl_rtl);
 	  if (modifier != EXPAND_CONST_ADDRESS
 	      && modifier != EXPAND_SUM
-	      && (!memory_address_p (DECL_MODE (exp), XEXP (decl_rtl, 0))
-		  || (flag_force_addr && !REG_P (XEXP (decl_rtl, 0)))))
+	      && !memory_address_p (DECL_MODE (exp), XEXP (decl_rtl, 0)))
 	    temp = replace_equiv_address (decl_rtl,
 					  copy_rtx (XEXP (decl_rtl, 0)));
 	}
@@ -7428,8 +7422,7 @@ expand_expr_real_1 (tree exp, rtx target
       if (modifier != EXPAND_CONST_ADDRESS
 	  && modifier != EXPAND_INITIALIZER
 	  && modifier != EXPAND_SUM
-	  && (! memory_address_p (mode, XEXP (temp, 0))
-	      || flag_force_addr))
+	  && ! memory_address_p (mode, XEXP (temp, 0)))
 	return replace_equiv_address (temp,
 				      copy_rtx (XEXP (temp, 0)));
       return temp;
@@ -9970,10 +9963,6 @@ do_tablejump (rtx index, enum machine_mo
     index = copy_to_mode_reg (Pmode, index);
 #endif
 
-  /* If flag_force_addr were to affect this address
-     it could interfere with the tricky assumptions made
-     about addresses that contain label-refs,
-     which may be valid only very near the tablejump itself.  */
   /* ??? The only correct use of CASE_VECTOR_MODE is the one inside the
      GET_MODE_SIZE, because this indicates how large insns are.  The other
      uses should all be Pmode, because they are addresses.  This code
@@ -9987,7 +9976,7 @@ do_tablejump (rtx index, enum machine_mo
     index = PIC_CASE_VECTOR_ADDRESS (index);
   else
 #endif
-    index = memory_address_noforce (CASE_VECTOR_MODE, index);
+    index = memory_address (CASE_VECTOR_MODE, index);
   temp = gen_reg_rtx (CASE_VECTOR_MODE);
   vector = gen_const_mem (CASE_VECTOR_MODE, index);
   convert_move (temp, vector, 0);
Index: expr.h
===================================================================
--- expr.h	(revision 130511)
+++ expr.h	(working copy)
@@ -616,9 +616,6 @@ extern rtx eliminate_constant_term (rtx,
    by emitting insns to perform arithmetic if nec.  */
 extern rtx memory_address (enum machine_mode, rtx);
 
-/* Like `memory_address' but pretend `flag_force_addr' is 0.  */
-extern rtx memory_address_noforce (enum machine_mode, rtx);
-
 /* Return a memory reference like MEMREF, but with its mode changed
    to MODE and its address changed to ADDR.
    (VOIDmode means don't change the mode.
Index: explow.c
===================================================================
--- explow.c	(revision 130511)
+++ explow.c	(working copy)
@@ -431,18 +431,21 @@ memory_address (enum machine_mode mode, 
 
       /* At this point, any valid address is accepted.  */
       if (memory_address_p (mode, x))
-	goto win;
+	goto done;
 
       /* If it was valid before but breaking out memory refs invalidated it,
 	 use it the old way.  */
       if (memory_address_p (mode, oldx))
-	goto win2;
+	{
+	  x = oldx;
+	  goto done;
+	}
 
       /* Perform machine-dependent transformations on X
 	 in certain cases.  This is not necessary since the code
 	 below can handle all possible cases, but machine-dependent
 	 transformations can make better code.  */
-      LEGITIMIZE_ADDRESS (x, oldx, mode, win);
+      LEGITIMIZE_ADDRESS (x, oldx, mode, done);
 
       /* PLUS and MULT can appear in special ways
 	 as the result of attempts to make an address usable for indexing.
@@ -482,17 +485,6 @@ memory_address (enum machine_mode mode, 
 	 the register is a valid address.  */
       else
 	x = force_reg (Pmode, x);
-
-      goto done;
-
-    win2:
-      x = oldx;
-    win:
-      if (flag_force_addr && ! cse_not_expected && !REG_P (x))
-	{
-	  x = force_operand (x, NULL_RTX);
-	  x = force_reg (Pmode, x);
-	}
     }
 
  done:
@@ -515,20 +507,6 @@ memory_address (enum machine_mode mode, 
   return x;
 }
 
-/* Like `memory_address' but pretend `flag_force_addr' is 0.  */
-
-rtx
-memory_address_noforce (enum machine_mode mode, rtx x)
-{
-  int ambient_force_addr = flag_force_addr;
-  rtx val;
-
-  flag_force_addr = 0;
-  val = memory_address (mode, x);
-  flag_force_addr = ambient_force_addr;
-  return val;
-}
-
 /* Convert a mem ref into one with a valid memory address.
    Pass through anything else unchanged.  */
 
@@ -538,8 +516,7 @@ validize_mem (rtx ref)
   if (!MEM_P (ref))
     return ref;
   ref = use_anchored_address (ref);
-  if (! (flag_force_addr && CONSTANT_ADDRESS_P (XEXP (ref, 0)))
-      && memory_address_p (GET_MODE (ref), XEXP (ref, 0)))
+  if (memory_address_p (GET_MODE (ref), XEXP (ref, 0)))
     return ref;
 
   /* Don't alter REF itself, since that is probably a stack slot.  */
Index: common.opt
===================================================================
--- common.opt	(revision 130511)
+++ common.opt	(working copy)
@@ -482,11 +482,9 @@ ffloat-store
 Common Report Var(flag_float_store) Optimization
 Don't allocate floats and doubles in extended-precision registers
 
-; Nonzero for -fforce-addr: load memory address into a register before
-; reference to memory.  This makes better cse but slower compilation.
 fforce-addr
-Common Report Var(flag_force_addr) Optimization
-Copy memory address constants into registers before use
+Common
+Does nothing.  Preserved for backward compatibility.
 
 fforward-propagate
 Common Report Var(flag_forward_propagate) Optimization
Index: combine.c
===================================================================
--- combine.c	(revision 130511)
+++ combine.c	(working copy)
@@ -1694,7 +1694,7 @@ can_combine_p (rtx insn, rtx i3, rtx pre
 	 change whether the life span of some REGs crosses calls or not,
 	 and it is a pain to update that information.
 	 Exception: if source is a constant, moving it later can't hurt.
-	 Accept that special case, because it helps -fforce-addr a lot.  */
+	 Accept that as a special case.  */
       || (DF_INSN_LUID (insn) < last_call_luid && ! CONSTANT_P (src)))
     return 0;
 
Index: config/cris/cris.h
===================================================================
--- config/cris/cris.h	(revision 130511)
+++ config/cris/cris.h	(working copy)
@@ -155,11 +155,16 @@ extern int cris_cpu_version;
       %{!fno-function-sections: -ffunction-sections}\
       %{!fno-data-sections: -fdata-sections}}}}"
 
-/* This adds to CC1_SPEC.  When bugs are removed from -fvtable-gc
-   (-fforce-addr causes invalid .vtable_entry asm in tinfo.cc and
-   nothing at all works in GCC 3.0-pre), add this line:
+/* This adds to CC1_SPEC.
+   FIXME: There used to be bugs with -fvtable-gc in combination with
+   -fforce-addr, and the latter was enabled by default for CRIS.  It
+   was suggested, in an old comment here, that the following line should
+   be added here once those bugs would be resolved:
+
    "%{mbest-lib-options:%{!moverride-best-lib-options:\
-   %{!melinux:%{!maout|melf:%{!fno-vtable-gc:-fvtable-gc}}}}}".  */
+   %{!melinux:%{!maout|melf:%{!fno-vtable-gc:-fvtable-gc}}}}}".
+
+   This could be tried agian, now that -fforce-addr has been removed.  */
 #define CC1PLUS_SPEC ""
 
 #ifdef HAVE_AS_NO_MUL_BUG_ABORT_OPTION
@@ -296,19 +301,14 @@ extern int target_flags;
 
 #define OVERRIDE_OPTIONS cris_override_options ()
 
-/* The following gives optimal code for gcc-2.7.2, but *may* be subject
-   to change.  Omitting flag_force_addr gives .1-.7% faster code for gcc
-   *only*, but 1.3% larger code.  On ipps it gives 5.3-10.6% slower
-   code(!) and 0.3% larger code.  For products, images gets .1-1.8%
-   larger.  Do not set strict aliasing from optimization options.  */
+/* FIXME: This once was tuned for gcc-2.7.2, but old flags have gone and
+   new flags have been added.  It may be worthwhile to try and find a new
+   set of optimizations options for cris, e.g. using ACOVEA.  */
 #define OPTIMIZATION_OPTIONS(OPTIMIZE, SIZE)	\
   do						\
     {						\
       if ((OPTIMIZE) >= 2 || (SIZE))		\
-	{					\
-	  flag_force_addr = 1;			\
-	  flag_omit_frame_pointer = 1;		\
-	}					\
+	flag_omit_frame_pointer = 1;		\
     }						\
   while (0)
 
Index: config/m68k/m68k.h
===================================================================
--- config/m68k/m68k.h	(revision 130511)
+++ config/m68k/m68k.h	(working copy)
@@ -755,9 +755,7 @@ __transfer_from_trampoline ()					\
     }									\
   while (0)
 
-/* Don't call memory_address_noforce for the address to fetch
-   the switch offset.  This address is ok as it stands,
-   but memory_address_noforce would alter it.  */
+/* This address is OK as it stands.  */
 #define PIC_CASE_VECTOR_ADDRESS(index) index
 \f
 /* For the 68000, we handle X+REG by loading X into a register R and

Re: PR33713: remove -fforce-addr

[Richard Guenther]

On Nov 29, 2007 6:50 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Hi folks.
>
> I'm picking up this patch that Steven attached to the PR but didn't
> submit to the list.  The patch removes support for -fforce-addr.
>
>         http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33713
>
> I have added a changelog, incorporated the change suggested by Paolo,
> fixed a typo, and tested it on x86_64-linux.  There are no regressions.
>
> Is this OK for mainline?

This is ok.  But please also provide an entry for the 4.3 changes.html page.

Thanks,
Richard.

>         PR33713
>         * doc/invoke.texi: Remove -fforce-addr documentation.
>         * expr.c (emit_move_insn): Remove use of flag_force_addr.
>         (expand_expr_real_1): Same.
>         (do_tablejump): Same.
>         Call memory_address instead of memory_address_noforce.
>         * expr.h (memory_address_noforce): Remove prototype.
>         * explow.c (memory_address): Remove support for flag_force_addr.
>         (validize_mem): Same.
>         (memory_address_noforce): Remove.
>         * common.opt: Add dummy documentation for -fforce-addr.
>         * combine.c (can_combine_p): Remove -fforce-addr comment.
>         * config/cris/cris.h: Update CC1_SPEC comment with regards to
>         -fforce-addr.
>         (OPTIMIZATION_OPTIONS): Remove set of flag_force_addr.
>         * config/m68k/m68k.h (PIC_CASE_VECTOR_ADDRESS): Remove comment
>         relating to memory_address_noforce.
>
>         * gcc.c-torture/compile/20050802-1.c: Remove.
>         * gcc.c-torture/compile/20011113-1.c: Remove.

Re: PR33713: remove -fforce-addr

[Michael Meissner]

On Thu, Nov 29, 2007 at 12:50:51PM -0500, Aldy Hernandez wrote:
> Hi folks.
> 
> I'm picking up this patch that Steven attached to the PR but didn't
> submit to the list.  The patch removes support for -fforce-addr.
> 
> 	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33713
> 
> I have added a changelog, incorporated the change suggested by Paolo,
> fixed a typo, and tested it on x86_64-linux.  There are no regressions.
> 
> Is this OK for mainline?
> 
> 	PR33713
> 	* doc/invoke.texi: Remove -fforce-addr documentation.
> 	* expr.c (emit_move_insn): Remove use of flag_force_addr.
> 	(expand_expr_real_1): Same.
> 	(do_tablejump): Same.
> 	Call memory_address instead of memory_address_noforce.
> 	* expr.h (memory_address_noforce): Remove prototype.
> 	* explow.c (memory_address): Remove support for flag_force_addr.
> 	(validize_mem): Same.
> 	(memory_address_noforce): Remove.
> 	* common.opt: Add dummy documentation for -fforce-addr.
> 	* combine.c (can_combine_p): Remove -fforce-addr comment.
> 	* config/cris/cris.h: Update CC1_SPEC comment with regards to
> 	-fforce-addr.
> 	(OPTIMIZATION_OPTIONS): Remove set of flag_force_addr.
> 	* config/m68k/m68k.h (PIC_CASE_VECTOR_ADDRESS): Remove comment
> 	relating to memory_address_noforce.
> 
> 	* gcc.c-torture/compile/20050802-1.c: Remove.
> 	* gcc.c-torture/compile/20011113-1.c: Remove.

I'm coming in late to this.  Given there were a few examples that were faster
with -fforce-addr, do we want to remove it without trying to fix the code?  I
didn't see from the bug report that the slowdowns had been addressed.

I also would prefer a deprecated switch get a warning, rather than silently
accepting it.

-- 
Michael Meissner, AMD
90 Central Street, MS 83-29, Boxborough, MA, 01719, USA
michael.meissner@amd.com

Re: PR33713: remove -fforce-addr

[Paolo Bonzini]

> I'm coming in late to this.  Given there were a few examples that were faster
> with -fforce-addr

There was basically one -- equake from SPEC2000.  I can try to look at 
it but I can't promise anything on the time.

> I also would prefer a deprecated switch get a warning, rather than silently
> accepting it.

This was the current practice, AFAICT.

Paolo

Re: PR33713: remove -fforce-addr

[Steven Bosscher]

On Nov 30, 2007 1:02 AM, Michael Meissner <michael.meissner@amd.com> wrote:
> I'm coming in late to this.

You could say that yes, especially since bugs with -fforce-addr have
existed for years.


>  Given there were a few examples that were faster
> with -fforce-addr, do we want to remove it without trying to fix the code?

Why would you want to fix this?  This is just another flag that is not
enabled by default. It usually (almost always, actually) results in
worse code if enabled.

So there are a few exceptions, where enabling -fflag-addr sometimes
still gives slightly better code.  But that is no different from e.g.
merges from branches (tree-ssa, df), or the removal of -fforce-mem,
which also caused small slowdowns for some benchmarks, but with an
improvemed overall score.  Those changes affected the generated code
with the default flags.  I didn't hear you (or anyone else) question
those patches.

So your question looks like a bikeshed to me.

But maybe you are volunteering? ;-)


> I didn't see from the bug report that the slowdowns had been addressed.

Which slowdowns?  This flag is not enabled by default.  Almost all
SPEC benchmarks, at least, slow down with -fforce-addr.  Nothing slows
down with the default options.  In fact nothing even *changes* at all
with the default options.

Stop spreading FUD.


> I also would prefer a deprecated switch get a warning, rather than silently
> accepting it.

Silently accepting is what is done for other removed flags too.  See
http://gcc.gnu.org/ml/gcc-patches/2006-02/msg01949.html.  You're too
late for that discussion, too :-P

Gr.
Steven

Re: PR33713: remove -fforce-addr

[Richard Guenther]

On Thu, 29 Nov 2007, Michael Meissner wrote:

> On Thu, Nov 29, 2007 at 12:50:51PM -0500, Aldy Hernandez wrote:
> > Hi folks.
> > 
> > I'm picking up this patch that Steven attached to the PR but didn't
> > submit to the list.  The patch removes support for -fforce-addr.
> > 
> > 	http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33713
> > 
> > I have added a changelog, incorporated the change suggested by Paolo,
> > fixed a typo, and tested it on x86_64-linux.  There are no regressions.
> > 
> > Is this OK for mainline?
> > 
> > 	PR33713
> > 	* doc/invoke.texi: Remove -fforce-addr documentation.
> > 	* expr.c (emit_move_insn): Remove use of flag_force_addr.
> > 	(expand_expr_real_1): Same.
> > 	(do_tablejump): Same.
> > 	Call memory_address instead of memory_address_noforce.
> > 	* expr.h (memory_address_noforce): Remove prototype.
> > 	* explow.c (memory_address): Remove support for flag_force_addr.
> > 	(validize_mem): Same.
> > 	(memory_address_noforce): Remove.
> > 	* common.opt: Add dummy documentation for -fforce-addr.
> > 	* combine.c (can_combine_p): Remove -fforce-addr comment.
> > 	* config/cris/cris.h: Update CC1_SPEC comment with regards to
> > 	-fforce-addr.
> > 	(OPTIMIZATION_OPTIONS): Remove set of flag_force_addr.
> > 	* config/m68k/m68k.h (PIC_CASE_VECTOR_ADDRESS): Remove comment
> > 	relating to memory_address_noforce.
> > 
> > 	* gcc.c-torture/compile/20050802-1.c: Remove.
> > 	* gcc.c-torture/compile/20011113-1.c: Remove.
> 
> I'm coming in late to this.  Given there were a few examples that were faster
> with -fforce-addr, do we want to remove it without trying to fix the code?  I
> didn't see from the bug report that the slowdowns had been addressed.

As it makes us reject valid code this is an easy decision.  Also
somebody performed SPEC meansurement and showed that without -fforce-addr
it is actually faster.

Can you give pointers to the few examples that were faster with 
-fforce-addr (and that are not only attributed to our bad register
allocator)?

> I also would prefer a deprecated switch get a warning, rather than silently
> accepting it.

In general I agree here, but I don't have a strong opinion for 
optimization switches.

Thanks,
Richard.

Re: PR33713: remove -fforce-addr

[Steven Bosscher]

On Nov 30, 2007 11:06 AM, Richard Guenther <rguenther@suse.de> wrote:
> > I also would prefer a deprecated switch get a warning, rather than silently
> > accepting it.
>
> In general I agree here, but I don't have a strong opinion for
> optimization switches.

The problem with a warning is that people might be building with
-Werror.  If they do that and they also have the deprecated flag in
their Makefile, their builds fail.

But telling users that their build flags have no effect is IMHO a good thing.

Proposal: We could add a "Deprecated" option property to common.opt,
and use "inform()" to tell users they are using a deprecated option.
If this sounds acceptable to y'all, I will craft a patch to implement
it.

(I couldn't find if this was proposed before.  "inform()" is quite
new, and I don't know if it was around already when it was first
discussed what to do with removed options.)

Gr.
Steven

Re: PR33713: remove -fforce-addr

[Hans-Peter Nilsson]

On Thu, 29 Nov 2007, Aldy Hernandez wrote:

> 	* config/cris/cris.h: Update CC1_SPEC comment with regards to
> 	-fforce-addr.

Please leave the comments, I'll update them.
thanks.

(Now, there are those -fforce-addr figures again?)

brgds, H-P

Re: PR33713: remove -fforce-addr

[Aldy Hernandez]

I have left the comments to the Cris back-end, as suggested by Hans, and
have committed the patch to mainline.

I will be writing up a 4.3 changes.html patch as a followup.

Aldy

> Proposal: We could add a "Deprecated" option property to common.opt,
> and use "inform()" to tell users they are using a deprecated option.
> If this sounds acceptable to y'all, I will craft a patch to implement
> it.
> 
> (I couldn't find if this was proposed before.  "inform()" is quite
> new, and I don't know if it was around already when it was first
> discussed what to do with removed options.)

Re: PR33713: remove -fforce-addr (changes.html patch)

[Aldy Hernandez]

On Thu, Nov 29, 2007 at 10:33:59PM +0100, Richard Guenther wrote:
> On Nov 29, 2007 6:50 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > Hi folks.
> >
> > I'm picking up this patch that Steven attached to the PR but didn't
> > submit to the list.  The patch removes support for -fforce-addr.
> >
> >         http://gcc.gnu.org/bugzilla/show_bug.cgi?id=33713
> >
> > I have added a changelog, incorporated the change suggested by Paolo,
> > fixed a typo, and tested it on x86_64-linux.  There are no regressions.
> >
> > Is this OK for mainline?
> 
> This is ok.  But please also provide an entry for the 4.3 changes.html page.

Is this OK?

Index: changes.html
===================================================================
RCS file: /cvs/gcc/wwwdocs/htdocs/gcc-4.3/changes.html,v
retrieving revision 1.87
diff -p -r1.87 changes.html
*** changes.html        28 Nov 2007 01:22:19 -0000      1.87
--- changes.html        3 Dec 2007 23:17:27 -0000
***************
*** 585,590 ****
--- 585,592 ----
        <li><code>-mno-short</code>, <code>-mno-bitfield</code> and
          <code>-mno-rtd</code> are now accepted as negative versions
          of <code>-mshort</code>, etc.</li>
+       <li><code>-fforce-addr</code> has been removed.  It is now
ignored
+           by the compiler.</li>
      </ul>
    <h4>Other improvements</h4>
      <ul>

Re: PR33713: remove -fforce-addr (changes.html patch)

[Gerald Pfeifer]

On Mon, 3 Dec 2007, Aldy Hernandez wrote:
> Is this OK?

Looks good, thanks!

Gerald

Re: PR33713: remove -fforce-addr

[Aldy Hernandez]

> I'll have a look myself.  I just want to state for the record
> that this change indeed caused regressions.

I only tested on x86-linux.

Thanks for taking a look at this.

> (No need for anyone to tell me that the change probably exposed
> a bug elsewhere, but thank you for caring.)

:)

Aldy

Re: PR33713: remove -fforce-addr

[Hans-Peter Nilsson]

> Date: Tue, 4 Dec 2007 11:19:26 -0400
> From: Aldy Hernandez <aldyh@redhat.com>

> > I'll have a look myself.  I just want to state for the record
> > that this change indeed caused regressions.
> 
> I only tested on x86-linux.

Wot?  First you said x86_64-linux! ;)

Anyway, the point of this communication: was it really a good
thing to remove all test-cases using -fforce-addr?  Why not just
remove their -fforce-addr argument?  Can we add them back?

brgds, H-P

Re: PR33713: remove -fforce-addr

[Aldy Hernandez]

> > I only tested on x86-linux.
> 
> Wot?  First you said x86_64-linux! ;)

You're reading what I write, not what I mean :).  Yeah, x86_64-linux, as
that's my primary workstation.

> Anyway, the point of this communication: was it really a good
> thing to remove all test-cases using -fforce-addr?  Why not just
> remove their -fforce-addr argument?  Can we add them back?

I don't have a problem with that.  Steven?

Re: PR33713: remove -fforce-addr

[Rask Ingemann Lambertsen]

On Thu, Nov 29, 2007 at 01:50:51PM -0400, Aldy Hernandez wrote:

> Index: explow.c
> ===================================================================
> --- explow.c	(revision 130511)
> +++ explow.c	(working copy)
[...]
> @@ -482,17 +485,6 @@ memory_address (enum machine_mode mode, 
>  	 the register is a valid address.  */
>        else
>  	x = force_reg (Pmode, x);
> -
> -      goto done;
> -
> -    win2:
> -      x = oldx;
> -    win:
> -      if (flag_force_addr && ! cse_not_expected && !REG_P (x))
> -	{
> -	  x = force_operand (x, NULL_RTX);
> -	  x = force_reg (Pmode, x);
> -	}
>      }
>  
>   done:

   This breaks at least m32c-unknown-elf:

make[2]: Entering directory `/home/rask/build/gcc-m32c-unknown-elf/gcc'
gcc -c   -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -fno-common   -DHAVE_CONFIG_H -I. -I. -I/home/rask/src/all/gcc -I/home/rask/src/all/gcc/. -I/home/rask/src/all/gcc/../include -I/home/rask/src/all/gcc/../libcpp/include  -I/home/rask/src/all/gcc/../libdecnumber -I/home/rask/src/all/gcc/../libdecnumber/dpd -I../libdecnumber    /home/rask/src/all/gcc/explow.c -o explow.o
/home/rask/src/all/gcc/explow.c: In function memory_address:
/home/rask/src/all/gcc/explow.c:448: error: label win used but not defined
make[2]: *** [explow.o] Error 1

-- 
Rask Ingemann Lambertsen
Danish law requires addresses in e-mail to be logged and stored for a year

Re: PR33713: remove -fforce-addr

[Aldy Hernandez]

>    This breaks at least m32c-unknown-elf:
> 
> make[2]: Entering directory `/home/rask/build/gcc-m32c-unknown-elf/gcc'
> gcc -c   -g -O2 -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE  -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -Wold-style-definition -Wmissing-format-attribute -fno-common   -DHAVE_CONFIG_H -I. -I. -I/home/rask/src/all/gcc -I/home/rask/src/all/gcc/. -I/home/rask/src/all/gcc/../include -I/home/rask/src/all/gcc/../libcpp/include  -I/home/rask/src/all/gcc/../libdecnumber -I/home/rask/src/all/gcc/../libdecnumber/dpd -I../libdecnumber    /home/rask/src/all/gcc/explow.c -o explow.o
> /home/rask/src/all/gcc/explow.c: In function memory_address:
> /home/rask/src/all/gcc/explow.c:448: error: label win used but not defined

m32c's LEGITIMIZE_ADDRESS is wrong.  It should be using the WIN macro,
not jumping to "win" directly, especially since the "win" label is no
longer present.

LEGITIMIZE_RELOAD_ADDRESS suffers from the same problem even though it's
not causing any problems now.

I've fixed both, and committed as obvious.

Aldy

	* m32c.h (LEGITIMIZE_ADDRESS): Use WIN instead of win.
	(LEGITIMIZE_RELOAD_ADDRESS): Same.

--- config/m32c/m32c.h	(revision 130686)
+++ config/m32c/m32c.h	(local)
@@ -561,11 +561,11 @@ typedef struct m32c_cumulative_args
 
 #define LEGITIMIZE_ADDRESS(X,OLDX,MODE,WIN) \
 	if (m32c_legitimize_address(&(X),OLDX,MODE)) \
-	  goto win;
+	  goto WIN;
 
 #define LEGITIMIZE_RELOAD_ADDRESS(X,MODE,OPNUM,TYPE,IND_LEVELS,WIN) \
 	if (m32c_legitimize_reload_address(&(X),MODE,OPNUM,TYPE,IND_LEVELS)) \
-	  goto win;
+	  goto WIN;

Re: PR33713: remove -fforce-addr

[DJ Delorie]

> I've fixed both, and committed as obvious.

Thanks!

Re: PR33713: remove -fforce-addr

[Rask Ingemann Lambertsen]

On Fri, Dec 07, 2007 at 12:48:18PM -0400, Aldy Hernandez wrote:
> 
> 	* m32c.h (LEGITIMIZE_ADDRESS): Use WIN instead of win.
> 	(LEGITIMIZE_RELOAD_ADDRESS): Same.

   That fixes it. Thanks.

-- 
Rask Ingemann Lambertsen
Danish law requires addresses in e-mail to be logged and stored for a year

Re: PR33713: remove -fforce-addr

[Hans-Peter Nilsson]

> Date: Tue, 4 Dec 2007 19:13:35 -0400
> From: Aldy Hernandez <aldyh@redhat.com>

> > Anyway, the point of this communication: was it really a good
> > thing to remove all test-cases using -fforce-addr?  Why not just
> > remove their -fforce-addr argument?  Can we add them back?
> 
> I don't have a problem with that.  Steven?

Any news on this?

FWIW, the bug exposed by the sad loss of -fforce-addr ;) was
covered again by Jakub's fix for PR rtl-optimization/32636.

Or possibly fixed; that seems to have been a likely cause.

Though it also indicates that the loss of -fforce-addr required
another register, and the call-clobbered R9 (CRIS struct_value
reg), seemed free.  So, a code-quality regression.

brgds, H-P

Re: PR33713: remove -fforce-addr

[Steven Bosscher]

On Dec 14, 2007 2:57 AM, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
> > Date: Tue, 4 Dec 2007 19:13:35 -0400
> > From: Aldy Hernandez <aldyh@redhat.com>
>
> > > Anyway, the point of this communication: was it really a good
> > > thing to remove all test-cases using -fforce-addr?  Why not just
> > > remove their -fforce-addr argument?  Can we add them back?
> >
> > I don't have a problem with that.  Steven?
>
> Any news on this?

IMHO without -fforce-addr the test cases test nothing that isn't
already triggered by a gcc build or by another test case.  I see no
point in keeping aruond test cases that add no test coverage.

Gr.
Steven




Index: testsuite/gcc.c-torture/compile/20050802-1.c
===================================================================
--- testsuite/gcc.c-torture/compile/20050802-1.c	(revision 130511)
+++ testsuite/gcc.c-torture/compile/20050802-1.c	(working copy)
@@ -1,10 +0,0 @@
-/* PR 23196 */
-/* { dg-options "-fforce-addr" } */
-
-void foo()
-{
-  char c;
-
-  c |= 1;
-  bar(&c);
-}
Index: testsuite/gcc.dg/20011113-1.c
===================================================================
--- testsuite/gcc.dg/20011113-1.c	(revision 130511)
+++ testsuite/gcc.dg/20011113-1.c	(working copy)
@@ -1,16 +0,0 @@
-/* Copyright (C) 2001  Free Software Foundation.
-   by Hans-Peter Nilsson  <hp@axis.com>  */
-
-/* { dg-do compile } */
-/* { dg-options "-O2 -fforce-addr" } */
-
-const char foo[] = "fum";
-const struct fi
-{
-  const char *const in;
-  const char *const out;
-  const int flags;
-} fie[] = {
-  {"nw", " new", 0},
-  {"dl", foo, 1}
-};

Re: PR33713: remove -fforce-addr

[Hans-Peter Nilsson]

> Date: Fri, 14 Dec 2007 08:25:11 +0100
> From: "Steven Bosscher" <stevenb.gcc@gmail.com>

> On Dec 14, 2007 2:57 AM, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
> > > Date: Tue, 4 Dec 2007 19:13:35 -0400
> > > From: Aldy Hernandez <aldyh@redhat.com>
> >
> > > > Anyway, the point of this communication: was it really a good
> > > > thing to remove all test-cases using -fforce-addr?  Why not just
> > > > remove their -fforce-addr argument?  Can we add them back?
> > >
> > > I don't have a problem with that.  Steven?
> >
> > Any news on this?
> 
> IMHO without -fforce-addr the test cases test nothing that isn't
> already triggered by a gcc build or by another test case.  I see no
> point in keeping aruond test cases that add no test coverage.

You're quick with your dismissals.  When removing -fforce-addr,
code was changed that at least formerly was exercised by the
test-cases.  As a result of that change, the generated code
changed, so without further analysis (as to whether the
execution path now actually is exercised by *these* tests),
keeping the test-cases trivially make sense.  Agree?

brgds, H-P

Re: PR33713: remove -fforce-addr

[Steven Bosscher]

On Dec 14, 2007 12:51 PM, Hans-Peter Nilsson
<hans-peter.nilsson@axis.com> wrote:
> > Date: Fri, 14 Dec 2007 08:25:11 +0100
> > From: "Steven Bosscher" <stevenb.gcc@gmail.com>
>
> > On Dec 14, 2007 2:57 AM, Hans-Peter Nilsson <hans-peter.nilsson@axis.com> wrote:
> > > > Date: Tue, 4 Dec 2007 19:13:35 -0400
> > > > From: Aldy Hernandez <aldyh@redhat.com>
> > >
> > > > > Anyway, the point of this communication: was it really a good
> > > > > thing to remove all test-cases using -fforce-addr?  Why not just
> > > > > remove their -fforce-addr argument?  Can we add them back?
> > > >
> > > > I don't have a problem with that.  Steven?
> > >
> > > Any news on this?
> >
> > IMHO without -fforce-addr the test cases test nothing that isn't
> > already triggered by a gcc build or by another test case.  I see no
> > point in keeping aruond test cases that add no test coverage.
>
> You're quick with your dismissals.  When removing -fforce-addr,
> code was changed that at least formerly was exercised by the
> test-cases.  As a result of that change, the generated code
> changed, so without further analysis (as to whether the
> execution path now actually is exercised by *these* tests),
> keeping the test-cases trivially make sense.  Agree?

No, not to me.

But to be honest, I don't want to even be in this typical bikeshed gcc
discussion.  If you feel so strongly about this, why don't you just
restore the test cases?

Gr.
Steven