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]

[dataflow]: PATCH COMMITTED to fix bugs.


This patch is two parts. 

The first part is by stevenb and is a duplicate of a patch submitted to
mainline.  On mainline this appears to be latent but on the dataflow
branch causes a regression on the x86-64.  Barriers should no longer be
emitted by cse since it is now done in cfglayout mode. 

The second part most likely fixes a regression on x96-64 darwin.  I do
not have access to such a machine and found the bug by looking at dse
dump files.  This patch will be tested later by echristo. 

There were two issues discovered by looking at the files.  First, I was
not computing the parameters for canon_true_dependence exactly right. 
In particular, the mem_address parameter needs to be directly computed
from the mem, not from my forward substituted mems.  The second issue
was that we should never allow the (HARD) FRAME_POINTER to be
substituted into other registers.  This most likely resulted in missing
a few stores that could have been deleted.  The issue is that dse knows
that the frame is special and that stores go dead at the end of the
function.  So if you translate an address from the frame into the stack,
you give up this knowledge. 

This patch has been tested on linux x86-64, x86-32, ppc, and ia-64.

Kenny

2007-04-12  Steven Bosscher  <steven@gcc.gnu.org>
        Kenneth Zadeck <zadeck@naturalbridge.com>

    * cse.c (cse_insn): Do not emit barriers.
    * dse.c (store_info.address): Renamed to mem_addr.
    (canon_address): Removed address_out parameter.
    (record_store): Removed address var and compute mem and
    mem_address differently.
    (check_mem_read_rtx): Removed address and changed parameters to
    canon_true_dependence.
    * cselib.c (cselib_expand_value_rtx): Do not translate
    FRAME_POINTER_REGNUM and HARD_FRAME_POINTER_REGNUM.
   
Index: cse.c
===================================================================
--- cse.c	(revision 123724)
+++ cse.c	(working copy)
@@ -5058,11 +5058,6 @@ cse_insn (rtx insn, rtx libcall_insn)
       else if (dest == pc_rtx && GET_CODE (src) == LABEL_REF
 	       && !LABEL_REF_NONLOCAL_P (src))
 	{
-	  /* Now emit a BARRIER after the unconditional jump.  */
-	  if (NEXT_INSN (insn) == 0
-	      || !BARRIER_P (NEXT_INSN (insn)))
-	    emit_barrier_after (insn);
-
 	  /* We reemit the jump in as many cases as possible just in
 	     case the form of an unconditional jump is significantly
 	     different than a computed jump or conditional jump.
@@ -5088,11 +5083,6 @@ cse_insn (rtx insn, rtx libcall_insn)
 
 	      delete_insn_and_edges (insn);
 	      insn = new;
-
-	      /* Now emit a BARRIER after the unconditional jump.  */
-	      if (NEXT_INSN (insn) == 0
-		  || !BARRIER_P (NEXT_INSN (insn)))
-		emit_barrier_after (insn);
 	    }
 	  else
 	    INSN_CODE (insn) = -1;
Index: dse.c
===================================================================
--- dse.c	(revision 123724)
+++ dse.c	(working copy)
@@ -211,11 +211,11 @@ struct store_info {
   /* This is the cselib value.  */
   cselib_val *cse_base;
 
-  /* This original mem.  */
+  /* This canonized mem.  */
   rtx mem;
 
-  /* This canonized address of mem.  */
-  rtx address;
+  /* The result of get_addr on mem.  */
+  rtx mem_addr;
 
   /* If this is non-zero, it is the alias set of a spill location.  */
   HOST_WIDE_INT alias_set;
@@ -913,10 +913,12 @@ add_wild_read (bb_info_t bb_info)
 /* Take all reasonable action to put the address of MEM into the form 
    that we can do analysis on.  
 
-   The gold standard is to get the address into the form: ADDRESS +
-   OFFSET where ADDRESS is something that rtx_varies_p considers a
+   The gold standard is to get the address into the form: address +
+   OFFSET where address is something that rtx_varies_p considers a
    constant.  When we can get the address in this form, we can do
-   global analysis on it.  
+   global analysis on it.  Note that for constant bases, address is
+   not actually returned, only the group_id.  The address can be
+   obtained from that.
 
    If that fails, we try cselib to get a value we can at least use
    locally.  If that fails we return false.  
@@ -929,7 +931,7 @@ add_wild_read (bb_info_t bb_info)
 static bool
 canon_address (bool for_read, bb_info_t bb_info, rtx mem,
 	       HOST_WIDE_INT *alias_set_out,
-	       rtx *address_out, int *group_id,
+	       int *group_id,
 	       HOST_WIDE_INT *offset, 
 	       cselib_val **base)
 {
@@ -996,7 +998,6 @@ canon_address (bool for_read, bb_info_t 
 
   /* Split the address into canonical BASE + OFFSET terms.  */
   address = canon_rtx (simplified_address);
-  *address_out = address;
 
   *offset = 0;
 
@@ -1061,7 +1062,6 @@ static int
 record_store (rtx body, bb_info_t bb_info)
 {
   rtx mem;
-  rtx address = NULL;
   HOST_WIDE_INT offset = 0;
   HOST_WIDE_INT width = 0;
   HOST_WIDE_INT spill_alias_set;
@@ -1114,7 +1114,7 @@ record_store (rtx body, bb_info_t bb_inf
       insn_info->cannot_delete = true;
 
   if (!canon_address (false, bb_info, mem, 
-		      &spill_alias_set, &address, &group_id, &offset, &base))
+		      &spill_alias_set, &group_id, &offset, &base))
     return 0;
 
   width = GET_MODE_SIZE (GET_MODE (mem));
@@ -1240,9 +1240,9 @@ record_store (rtx body, bb_info_t bb_inf
   /* Finish filling in the store_info.  */
   store_info->next = insn_info->store_rec;
   insn_info->store_rec = store_info;
-  store_info->mem = mem;
+  store_info->mem = canon_rtx (mem);
   store_info->alias_set = spill_alias_set;
-  store_info->address = address;
+  store_info->mem_addr = get_addr (XEXP (mem, 0));
   store_info->cse_base = base;
   store_info->positions_needed = sbitmap_alloc (width);
   sbitmap_ones (store_info->positions_needed);
@@ -1275,7 +1275,6 @@ static int
 check_mem_read_rtx (rtx *loc, void *data)
 {
   rtx mem = *loc;
-  rtx address;
   bb_info_t bb_info;
   insn_info_t insn_info;
   HOST_WIDE_INT offset = 0;
@@ -1310,7 +1309,7 @@ check_mem_read_rtx (rtx *loc, void *data
     return 0;
 
   if (!canon_address (true, bb_info, mem, 
-		      &spill_alias_set, &address, &group_id, &offset, &base))
+		      &spill_alias_set, &group_id, &offset, &base))
     {
       if (dump_file)
 	fprintf (dump_file, " adding wild read, canon_address failure.\n");
@@ -1400,7 +1399,7 @@ check_mem_read_rtx (rtx *loc, void *data
 	    remove 
 	      = canon_true_dependence (store_info->mem, 
 				       GET_MODE (store_info->mem),
-				       store_info->address,
+				       store_info->mem_addr,
 				       mem, rtx_varies_p);
 	  
 	  else if (group_id == store_info->group_id)
@@ -1411,7 +1410,7 @@ check_mem_read_rtx (rtx *loc, void *data
 		remove 
 		  = canon_true_dependence (store_info->mem, 
 					   GET_MODE (store_info->mem),
-					   store_info->address,
+					   store_info->mem_addr,
 					   mem, rtx_varies_p);
 	      
 	      /* The bases are the same, just see if the offsets
@@ -1468,7 +1467,7 @@ check_mem_read_rtx (rtx *loc, void *data
 	  if (!store_info->alias_set)
 	    remove = canon_true_dependence (store_info->mem, 
 					    GET_MODE (store_info->mem),
-					    store_info->address,
+					    store_info->mem_addr,
 					    mem, rtx_varies_p);
 	  
 	  if (remove)
Index: cselib.c
===================================================================
--- cselib.c	(revision 123724)
+++ cselib.c	(working copy)
@@ -923,18 +923,25 @@ cselib_expand_value_rtx (rtx orig, bitma
 	      rtx result;
 	      int regno = REGNO (orig);
 	      
-	      /* The one thing that we are not willing to do (and this
-		 is requirement of dse and if others need this
-		 function we should add a parm to control it) is that
-		 we will not substitute the STACK_POINTER_REGNUM.  
+	      /* The several thing that we are not willing to do (this
+		 is requirement of dse and if others potiential uses
+		 need this function we should add a parm to control
+		 it) is that we will not substitute the
+		 STACK_POINTER_REGNUM, FRAME_POINTER or the
+		 HARD_FRAME_POINTER.
 
-		 Such an expansion confuses the code that notices that
-		 stores go dead to the frame at the end of the
+		 Thses expansions confuses the code that notices that
+		 stores into the frame go dead at the end of the
 		 function and that the frame is not effected by calls
-		 to subroutines.  If you allow the substitution, then
-		 the dse code will think that parameter pushing also
-		 goes dead.  */
-	      if (regno == STACK_POINTER_REGNUM)
+		 to subroutines.  If you allow the
+		 STACK_POINTER_REGNUM substitution, then dse will
+		 think that parameter pushing also goes dead which is
+		 wrong.  If you allow the FRAME_POINTER or the
+		 HARD_FRAME_POINTER then you loose the opportunity to
+		 make the frame assumptions.  */
+	      if (regno == STACK_POINTER_REGNUM
+		  || regno == FRAME_POINTER_REGNUM
+		  || regno == HARD_FRAME_POINTER_REGNUM)
 		return orig;
 
 	      bitmap_set_bit (regs_active, regno);

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