Re: Dataflow branch review #3

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

I did everything requested.  The only thing worth noting is that I
removed the continue in delete_noop_moves.  This was most likely
leftover residue from one of the many libcall notes debugging episodes. 
I then bootstrapped and regression tested on x86-32, x86-64, ppc and
ia-64 to make sure that the continue really was not needed. 

I also removed the nnoop counter from delete_noop_moves since the single
caller did not care.

Kenny


2007-06-06  Kenneth Zadeck <zadeck@naturalbridge.com>

    * combine.c (find_single_use_1): Fixed comment and changed 0 to NULL.
    (find_single_use): Removed comment.
    (delete_noop_move): Removed unnecessary continue and removed
    nnoops counter.  Function now does not return anything.


Ian Lance Taylor wrote:
> A few more notes on the dataflow branch.
>
> Ian
>
> In combine.c:
>
> +static rtx *find_single_use_1 (rtx, rtx *);
> +/* This is used by find_single_use to locate an rtx that contains exactly one
> +   use of DEST, which is typically either a REG or CC0.  It returns a
> +   pointer to the innermost rtx expression containing DEST.  Appearances of
> +   DEST that are being used to totally replace it are not counted.  */
> +
> +static rtx *
> +find_single_use_1 (rtx dest, rtx *loc)
> +{
> +  rtx x = *loc;
> +  enum rtx_code code = GET_CODE (x);
> +  rtx *result = 0;
> +  rtx *this_result;
> +  int i;
> +  const char *fmt;
>
> Mention the LOC parameter in the comment before the function.
> Initialize RESULT to NULL, not 0.
>
> +	  if (result == 0)
> +	    result = this_result;
> +	  else if (this_result)
> +	    /* Duplicate usage.  */
> +	    return 0;
>
> Compare against NULL, not 0.  Return NULL, not 0.  And likewise in the
> similar code immediately following.
>
> In general try to write this sort of function using for_each_rtx, but
> that can wait for later.
>
>
> Before the function find_single_use:
>
> +   This routine will return usually zero either before flow is called (because
> +   there will be no LOG_LINKS notes) or after reload (because the REG_DEAD
> +   note can't be trusted).
>
> This function is now static in combine.c, so this comment is
> meaningless and should be removed.
>
>
> In delete_noop_moves:
>
> +	      if ((note = find_reg_note (insn, REG_LIBCALL, NULL_RTX))
> +		       && XEXP (note, 0) != insn)
> +		{
> +		  rtx new_libcall_insn = next_real_insn (insn);
> +		  rtx retval_note = find_reg_note (XEXP (note, 0),
> +						   REG_RETVAL, NULL_RTX);
> +continue;
> +		  REG_NOTES (new_libcall_insn)
> +		    = gen_rtx_INSN_LIST (REG_LIBCALL, XEXP (note, 0),
> +					 REG_NOTES (new_libcall_insn));
> +		  XEXP (retval_note, 0) = new_libcall_insn;
> +		}
>
> That "continue" looks very wrong.  I think what you are trying to say
> is that a move insn with a REG_LIBCALL note should not be deleted.
> This needs to be cleaned up.
>   

Index: combine.c
===================================================================
--- combine.c	(revision 125210)
+++ combine.c	(working copy)
@@ -468,18 +468,18 @@ static rtx gen_lowpart_or_truncate (enum
 static const struct rtl_hooks combine_rtl_hooks = RTL_HOOKS_INITIALIZER;
 
 
-static rtx *find_single_use_1 (rtx, rtx *);
-/* This is used by find_single_use to locate an rtx that contains exactly one
-   use of DEST, which is typically either a REG or CC0.  It returns a
-   pointer to the innermost rtx expression containing DEST.  Appearances of
-   DEST that are being used to totally replace it are not counted.  */
+/* This is used by find_single_use to locate an rtx in LOC that
+   contains exactly one use of DEST, which is typically either a REG
+   or CC0.  It returns a pointer to the innermost rtx expression
+   containing DEST.  Appearances of DEST that are being used to
+   totally replace it are not counted.  */
 
 static rtx *
 find_single_use_1 (rtx dest, rtx *loc)
 {
   rtx x = *loc;
   enum rtx_code code = GET_CODE (x);
-  rtx *result = 0;
+  rtx *result = NULL;
   rtx *this_result;
   int i;
   const char *fmt;
@@ -536,11 +536,11 @@ find_single_use_1 (rtx dest, rtx *loc)
 	  else
 	    this_result = find_single_use_1 (dest, &XEXP (x, i));
 
-	  if (result == 0)
+	  if (result == NULL)
 	    result = this_result;
 	  else if (this_result)
 	    /* Duplicate usage.  */
-	    return 0;
+	    return NULL;
 	}
       else if (fmt[i] == 'E')
 	{
@@ -556,10 +556,10 @@ find_single_use_1 (rtx dest, rtx *loc)
 	      else
 		this_result = find_single_use_1 (dest, &XVECEXP (x, i, j));
 
-	      if (result == 0)
+	      if (result == NULL)
 		result = this_result;
 	      else if (this_result)
-		return 0;
+		return NULL;
 	    }
 	}
     }
@@ -574,10 +574,6 @@ find_single_use_1 (rtx dest, rtx *loc)
 
    If PLOC is nonzero, *PLOC is set to the insn containing the single use.
 
-   This routine will return usually zero either before flow is called (because
-   there will be no LOG_LINKS notes) or after reload (because the REG_DEAD
-   note can't be trusted).
-
    If DEST is cc0_rtx, we look only at the next insn.  In that case, we don't
    care about REG_DEAD notes or LOG_LINKS.
 
@@ -851,12 +847,11 @@ combine_validate_cost (rtx i1, rtx i2, r
 
 /* Delete any insns that copy a register to itself.  */
 
-static int
+static void
 delete_noop_moves (void)
 {
   rtx insn, next;
   basic_block bb;
-  int nnoops = 0;
 
   FOR_EACH_BB (bb)
     {
@@ -876,23 +871,19 @@ delete_noop_moves (void)
 		  rtx new_libcall_insn = next_real_insn (insn);
 		  rtx retval_note = find_reg_note (XEXP (note, 0),
 						   REG_RETVAL, NULL_RTX);
-continue;
 		  REG_NOTES (new_libcall_insn)
 		    = gen_rtx_INSN_LIST (REG_LIBCALL, XEXP (note, 0),
 					 REG_NOTES (new_libcall_insn));
 		  XEXP (retval_note, 0) = new_libcall_insn;
 		}
 
+	      if (dump_file)
+		fprintf (dump_file, "deleting noop move %d\n", INSN_UID (insn));
+
 	      delete_insn_and_edges (insn);
-	      nnoops++;
 	    }
 	}
     }
-
-  if (nnoops && dump_file)
-    fprintf (dump_file, "deleted %i noop moves\n", nnoops);
-
-  return nnoops;
 }