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]

[Fwd: Re: PR rtl-optimization/15248 -- semi-latent reload bug]


Grr, didn't hit Reply-All.


Bernd
--- Begin Message --- Ok, I had a look around this code. I'm worried about relying on MEM_READONLY_P - it seems to me that if a user has a regular non-const pointer to ROM, we can inadvertently produce writes to it even if the source code only ever does reads.

It appears that this code in reload1.c:

          /* If this register is being made equivalent to a MEM
             and the MEM is not SET_SRC, the equivalencing insn
             is one with the MEM as a SET_DEST and it occurs later.
             So don't mark this insn now.  */
          if (!MEM_P (x)
              || rtx_equal_p (SET_SRC (set), x))
            reg_equiv_init[i]
              = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv_init[i]);

isn't doing the right thing. As you pointed out, there can be cases where the SET_SRC isn't the MEM, but an intermediate pseudo, in which case we still want to eliminate this initializing insn.

I don't think we have to worry too much about PARALLELs when deciding whether a store can be deleted or not; see this code in local-alloc.c:

set = single_set (insn);

  /* If this insn contains more (or less) than a single SET,
     only mark all destinations as having no known equivalence.  */
  if (set == 0)
    {
      note_stores (PATTERN (insn), no_equiv, NULL);
      continue;
    }

So, I believe all equivalencing insns are going to be single_sets.

Now, I think update_equiv_regs only handles two cases.
1. A register used inside one basic block only, set only once,
   and written to memory, becomes REG_EQUIV to that location.
2. Register always set to the same value which is REG_EQUAL to
   something else.

Based on all that, I came up with the following patch. It's not necessarily the best way to fix it; it's minimal but does not make the logic very clear. Anyhow, try it on your testcase. It has survived an x86-linux bootstrap, but I've done no other testing.


Bernd
Index: reload1.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/reload1.c,v
retrieving revision 1.469
diff -c -p -r1.469 reload1.c
*** reload1.c	23 Apr 2005 21:27:49 -0000	1.469
--- reload1.c	9 May 2005 13:16:37 -0000
*************** reload (rtx first, int global)
*** 753,759 ****
  		     for equivalences.  This is overly conservative as
  		     we could find all sets of the destination pseudo
  		     and remove them as they should be redundant.  */
! 		  if (memory_operand (x, VOIDmode) && ! MEM_READONLY_P (x))
  		    {
  		      /* Always unshare the equivalence, so we can
  			 substitute into this insn without touching the
--- 753,759 ----
  		     for equivalences.  This is overly conservative as
  		     we could find all sets of the destination pseudo
  		     and remove them as they should be redundant.  */
! 		  if (memory_operand (x, VOIDmode))
  		    {
  		      /* Always unshare the equivalence, so we can
  			 substitute into this insn without touching the
*************** reload (rtx first, int global)
*** 788,801 ****
  		  else
  		    continue;
  
! 		  /* If this register is being made equivalent to a MEM
! 		     and the MEM is not SET_SRC, the equivalencing insn
! 		     is one with the MEM as a SET_DEST and it occurs later.
! 		     So don't mark this insn now.  */
! 		  if (!MEM_P (x)
! 		      || rtx_equal_p (SET_SRC (set), x))
! 		    reg_equiv_init[i]
! 		      = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv_init[i]);
  		}
  	    }
  	}
--- 788,795 ----
  		  else
  		    continue;
  
! 		  reg_equiv_init[i]
! 		    = gen_rtx_INSN_LIST (VOIDmode, insn, reg_equiv_init[i]);
  		}
  	    }
  	}
*************** reload (rtx first, int global)
*** 807,815 ****
  	       && reg_equiv_memory_loc[REGNO (SET_SRC (set))]
  	       && rtx_equal_p (SET_DEST (set),
  			       reg_equiv_memory_loc[REGNO (SET_SRC (set))]))
  	reg_equiv_init[REGNO (SET_SRC (set))]
! 	  = gen_rtx_INSN_LIST (VOIDmode, insn,
! 			       reg_equiv_init[REGNO (SET_SRC (set))]);
  
        if (INSN_P (insn))
  	scan_paradoxical_subregs (PATTERN (insn));
--- 801,812 ----
  	       && reg_equiv_memory_loc[REGNO (SET_SRC (set))]
  	       && rtx_equal_p (SET_DEST (set),
  			       reg_equiv_memory_loc[REGNO (SET_SRC (set))]))
+ 	/* Equivalences made this way only have one initializing insn.
+ 	   Previously, we may have set reg_equiv_init when encountering a
+ 	   SET of this pseudo; discard that insn since it does not set up
+ 	   an equivalence.  */
  	reg_equiv_init[REGNO (SET_SRC (set))]
! 	  = gen_rtx_INSN_LIST (VOIDmode, insn, NULL_RTX);
  
        if (INSN_P (insn))
  	scan_paradoxical_subregs (PATTERN (insn));

--- End Message ---

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