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]

[RFA:] Consistency check for reload. Bug modifying reg_equiv_memory_locfixed.


There are four book-keeping arrays in reload, indexed by
pseudo-register number: reg_equiv_constant, reg_equiv_memory_loc,
reg_equiv_address and reg_equiv_mem.  How are they supposed to
be handled regarding substitution during reload?  Reading the
code, it seems to me the contents is supposed to remain constant
during processing, after setup and until cleanup: the contents
of those arrays must not be touched by the reload substitutions.

If so, reload1.c and reload.c don't really do it like that.  At
least the array reg_equiv_memory_loc is modified for at least
the insn holding the REG_EQUIV note that caused the
reg_equiv_memory_loc entry.  For most cases this doesn't matter,
as the equivalence isn't used or the substitution (most often a
register) dies after all uses and is as usable as the original
equivalence.

But some cases aren't that benign.  Not including the insn with
the REG_EQUIV note, I'm seeing substitutions in reg_equiv_memory_loc
which cause incorrect code because the "equivalence" register
did not hold an equivalence any more...  This on an unfinished
port; mmix-knuth-mmixware (not in CVS; not really ready for
consumption).  Code triggering the supposed reload bug are 22
c-torture/execute tests, all at -O0.  I can see other new ports
also getting caught by similar corner cases in reload.  The pain
of tracing incorrect code...  Yay for the new register
allocator: reload be gone!

Adding consistency checks that we're not modifying those reload
arrays seems TRT.  The time needed for the extra processing was
not detectable on bootstrap-check on i686-pc-linux-gnu and
build-check on powerpc-unknown-eabisim (1:41h+-1minute and
1:37h+-1m respectively, before and after the patch).  Hence the
"grab-bag" ENABLE_CHECKING flag seems appropriate.  Memory use
due to the extra copies was not fully measured, but I counted a
total of 40704 extra rtx copies caused by the patches below,
during stage 3 (executing stage 2 to get results to compare
with) on i686-pc-linux-gnu.  Instrumentation was done by adding
a fputs call with a magic string next to those new copies and
doing "grep -c".  IIUC the bulk of the equivalences are of type
(plus reg const_int) with some (mem reg) and (mem (plus reg
const_int)); tens of bytes each.  So I think the memory overhead
for the patch is low enough to not matter.

An interesting detail is that the reload.c:make_memloc change
does not seem necessary for i686-pc-linux-gnu, but is for
powerpc-unknown-eabisim.  It also solves the problem for
mmix-knuth-mmixware.  Actually that specific change did not
trigger *at all* for the instrumented i686-pc-linux-gnu
bootstrap.  I would have expected that it would cause a few
redundant copies.

I'd guess that for some ports there are other reload bugs of
this class lurking; they will probably break with this new
check.  If you're on a system caught by this, send preprocessed
source and indication of failure after this patch.

Bootstrapped and checked on i686-pc-linux-gnu and
sparc-sun-solaris2.8.  Built and checked on
powerpc-unknown-eabisim, mips-elf/mips-sim and
mn10300-elf/mn10300-sim with no regressions (around 2001-07-23
when they all still built; as mentioned, I'm checking what
simulator combinations are usable for GCC testing).

BTW, depending on system load, compile/20001226-1.c times out,
at least on sparc-sun-solaris2.8.  This delayed the submission
of this patch while investigating whether the cause was due to
the patch.  During the course of that, I thoughtlessly updated
and got trapped in the recent brokenness.  Mentioned just as a
datapoint.

Ok to commit?

	* reload1.c (reload): Make all entries in reg_equiv_memory_loc
	unshared.
	* reload.c (make_memloc): Copy result if it is still
	reg_equiv_memory_loc[regno] on return.
	(subst_reloads) [ENABLE_CHECKING]: Check that none of
	reg_equiv_constant, reg_equiv_memory_loc, reg_equiv_address and
	reg_equiv_mem are modified by the substitutions.

Index: reload.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/reload.c,v
retrieving revision 1.151
diff -p -c -r1.151 reload.c
*** reload.c	2001/07/23 13:21:39	1.151
--- reload.c	2001/07/24 22:03:17
*************** make_memloc (ad, regno)
*** 4485,4491 ****
      tem = copy_rtx (tem);

    tem = replace_equiv_address_nv (reg_equiv_memory_loc[regno], tem);
!   return adjust_address_nv (tem, GET_MODE (ad), 0);
  }

  /* Record all reloads needed for handling memory address AD
--- 4485,4497 ----
      tem = copy_rtx (tem);

    tem = replace_equiv_address_nv (reg_equiv_memory_loc[regno], tem);
!   tem = adjust_address_nv (tem, GET_MODE (ad), 0);
!
!   /* Copy the result if it's still the same as the equivalence, to avoid
!      modifying it when we do the substitution for the reload.  */
!   if (tem == reg_equiv_memory_loc[regno])
!     tem = copy_rtx (tem);
!   return tem;
  }

  /* Record all reloads needed for handling memory address AD
*************** subst_reloads (insn)
*** 5768,5773 ****
--- 5774,5805 ----
        register rtx reloadreg = rld[r->what].reg_rtx;
        if (reloadreg)
  	{
+ #ifdef ENABLE_CHECKING
+ 	  /* Internal consistency test.  Check that we don't modify
+ 	     anything in the equivalence arrays.  Whenever something from
+ 	     those arrays needs to be reloaded, it must be unshared before
+ 	     being substituted into; the equivalence must not be modified.
+ 	     Otherwise, if the equivalence is used after that, it will
+ 	     have been modified, and the thing substituted (probably a
+ 	     register) is likely overwritten and not a usable equivalence.  */
+ 	  int check_regno;
+
+ 	  for (check_regno = 0; check_regno < max_regno; check_regno++)
+ 	    {
+ #define CHECK_MODF(ARRAY)						\
+ 	      if (ARRAY[check_regno]					\
+ 		  && loc_mentioned_in_p (r->where,			\
+ 					 ARRAY[check_regno]))		\
+ 		abort ()
+
+ 	      CHECK_MODF (reg_equiv_constant);
+ 	      CHECK_MODF (reg_equiv_memory_loc);
+ 	      CHECK_MODF (reg_equiv_address);
+ 	      CHECK_MODF (reg_equiv_mem);
+ #undef CHECK_MODF
+ 	    }
+ #endif /* ENABLE_CHECKING */
+
  	  /* If we're replacing a LABEL_REF with a register, add a
  	     REG_LABEL note to indicate to flow which label this
  	     register refers to.  */
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/reload1.c,v
retrieving revision 1.278
diff -p -c -r1.278 reload1.c
*** reload1.c	2001/07/19 19:46:29	1.278
--- reload1.c	2001/07/24 22:03:23
*************** reload (first, global)
*** 783,794 ****
  		{
  		  if (GET_CODE (x) == MEM)
  		    {
! 		      /* If the operand is a PLUS, the MEM may be shared,
! 			 so make sure we have an unshared copy here.  */
! 		      if (GET_CODE (XEXP (x, 0)) == PLUS)
! 			x = copy_rtx (x);
!
! 		      reg_equiv_memory_loc[i] = x;
  		    }
  		  else if (function_invariant_p (x))
  		    {
--- 783,792 ----
  		{
  		  if (GET_CODE (x) == MEM)
  		    {
! 		      /* Always unshare the equivalence, so we can
! 			 substitute into this insn without touching the
! 			 equivalence. */
! 		      reg_equiv_memory_loc[i] = copy_rtx (x);
  		    }
  		  else if (function_invariant_p (x))
  		    {

brgds, H-P



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