This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] Fix register elimination bug in reload


Hello,

this fixes a latent reload bug in elimination offset handling that causes
wrong-code generation on s390x-ibm-linux when building with -mpacked-stack
in certain cases.

The problem occurs if during the initial calculate_needs_all_insns loop
in reload, one of the find_reloads calls needs to force a constant to
the constant pool, and the function did not previously have a pool.  
In this case, it may be necessary to save/restore the pool base register
now, even though it wasn't initially.  With the packed stack layout,
this means that the raw frame size, and thus elimination offsets, have
now changed.

However, the main loop in reload checks only whether the 'middle-end'
frame size (holding locals, spill, etc.) has changed -- it does not
actually have any knowledge about the size of the register save area.
Thus it may happen that the main loop does not notice that something
has changed that affects elimination offsets.

Now, during the final reload_as_needed step, the back end is called
again to recompute the offsets.  However, the *label offsets* computed
earlier in calculate_needs_all_insns are *not* recomputed.  Thus, after
the first label, incorrect elimination offsets may be applied.

The test case below shows such a case, where the arg_pointer is incorrectly
eliminated.

As fix I'd suggest to simply query the back end at the end of the main
loop in reload again, and if the offsets have changed in any way, make
another pass through the loop.  To do so I'm simply reusing the existing
verify_initial_elim_offsets function, modified so that it doesn't abort
if the offsets don't match, but simply returns a status value.

Bootstrapped/regtested on s390-ibm-linux and s390x-ibm-linux on mainline
and the 4.0 branch.  OK to apply?

Bye,
Ulrich



ChangeLog:

	* reload1.c (verify_initial_elim_offsets): Return boolean status
	instead of aborting.
	(reload): Adapt verify_initial_elim_offsets call site.  Restart
	main loop if some initial elimination offsets changed.

testsuite/ChangeLog:

	* gcc.dg/20050524-1.c: New test.

Index: gcc/reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.469
diff -c -p -r1.469 reload1.c
*** gcc/reload1.c	23 Apr 2005 21:27:49 -0000	1.469
--- gcc/reload1.c	24 May 2005 20:02:21 -0000
*************** static int eliminate_regs_in_insn (rtx, 
*** 383,389 ****
  static void update_eliminable_offsets (void);
  static void mark_not_eliminable (rtx, rtx, void *);
  static void set_initial_elim_offsets (void);
! static void verify_initial_elim_offsets (void);
  static void set_initial_label_offsets (void);
  static void set_offsets_for_label (rtx);
  static void init_elim_table (void);
--- 383,389 ----
  static void update_eliminable_offsets (void);
  static void mark_not_eliminable (rtx, rtx, void *);
  static void set_initial_elim_offsets (void);
! static bool verify_initial_elim_offsets (void);
  static void set_initial_label_offsets (void);
  static void set_offsets_for_label (rtx);
  static void init_elim_table (void);
*************** reload (rtx first, int global)
*** 984,989 ****
--- 984,996 ----
        if (starting_frame_size != get_frame_size ())
  	something_changed = 1;
  
+       /* Even if the frame size remained the same, we might still have
+ 	 changed elimination offsets, e.g. if find_reloads called 
+ 	 force_const_mem requiring the back end to allocate a constant
+ 	 pool base register that needs to be saved on the stack.  */
+       else if (!verify_initial_elim_offsets ())
+ 	something_changed = 1;
+ 
        {
  	HARD_REG_SET to_spill;
  	CLEAR_HARD_REG_SET (to_spill);
*************** reload (rtx first, int global)
*** 1075,1082 ****
  
        gcc_assert (old_frame_size == get_frame_size ());
  
!       if (num_eliminable)
! 	verify_initial_elim_offsets ();
      }
  
    /* If we were able to eliminate the frame pointer, show that it is no
--- 1082,1088 ----
  
        gcc_assert (old_frame_size == get_frame_size ());
  
!       gcc_assert (verify_initial_elim_offsets ());
      }
  
    /* If we were able to eliminate the frame pointer, show that it is no
*************** mark_not_eliminable (rtx dest, rtx x, vo
*** 3300,3322 ****
     where something illegal happened during reload_as_needed that could
     cause incorrect code to be generated if we did not check for it.  */
  
! static void
  verify_initial_elim_offsets (void)
  {
    HOST_WIDE_INT t;
  
  #ifdef ELIMINABLE_REGS
    struct elim_table *ep;
  
    for (ep = reg_eliminate; ep < &reg_eliminate[NUM_ELIMINABLE_REGS]; ep++)
      {
        INITIAL_ELIMINATION_OFFSET (ep->from, ep->to, t);
!       gcc_assert (t == ep->initial_offset);
      }
  #else
    INITIAL_FRAME_POINTER_OFFSET (t);
!   gcc_assert (t == reg_eliminate[0].initial_offset);
  #endif
  }
  
  /* Reset all offsets on eliminable registers to their initial values.  */
--- 3306,3335 ----
     where something illegal happened during reload_as_needed that could
     cause incorrect code to be generated if we did not check for it.  */
  
! static bool
  verify_initial_elim_offsets (void)
  {
    HOST_WIDE_INT t;
  
+   if (!num_eliminable)
+     return true;
+ 
  #ifdef ELIMINABLE_REGS
    struct elim_table *ep;
  
    for (ep = reg_eliminate; ep < &reg_eliminate[NUM_ELIMINABLE_REGS]; ep++)
      {
        INITIAL_ELIMINATION_OFFSET (ep->from, ep->to, t);
!       if (t != ep->initial_offset)
! 	return false;
      }
  #else
    INITIAL_FRAME_POINTER_OFFSET (t);
!   if (t != reg_eliminate[0].initial_offset)
!     return false;
  #endif
+ 
+   return true;
  }
  
  /* Reset all offsets on eliminable registers to their initial values.  */
*** /dev/null	Tue Oct 26 21:09:21 2004
--- gcc/testsuite/gcc.dg/20050524-1.c	Tue May 24 22:12:40 2005
***************
*** 0 ****
--- 1,34 ----
+ /* This test case used to abort due to a reload bug with
+    elimination offsets.  */
+ 
+ /* { dg-do run { target s390*-*-* } } */
+ /* { dg-options "-O2 -mpacked-stack" } */
+ 
+ extern void abort (void);
+ 
+ double bar (double) __attribute__ ((noinline));
+ double bar (double x) { return x; }
+ 
+ double
+ foo (int j, double f0, double f2, double f4, double f6, double x) __attribute__ ((noinline));
+ 
+ double
+ foo (int j, double f0, double f2, double f4, double f6, double x)
+ {
+   if (j)
+     return bar (x) + 4.0;
+   else
+     return bar (x);
+ }
+ 
+ int
+ main (void)
+ {
+   if (foo (0, 0, 0, 0, 0, 10) != 10)
+     abort ();
+   if (foo (1, 0, 0, 0, 0, 10) != 14)
+     abort ();
+ 
+   return 0;
+ }
+ 
-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]