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]

Re: dead code elimination after constant propagation



--

Return-Path: <law@hurl.cygnus.com>
Received: (qmail 302 invoked from network); 6 Jan 1999 08:25:55 -0000
Received: from runyon.cygnus.com (HELO cygnus.com) (205.180.230.5)
  by egcs.cygnus.com with SMTP; 6 Jan 1999 08:25:55 -0000
Received: from localhost.cygnus.com (hurl.cygnus.com [208.224.120.146])
	by runyon.cygnus.com (8.8.7-cygnus/8.8.7) with ESMTP id AAA23597;
	Wed, 6 Jan 1999 00:25:38 -0800 (PST)
To: Christian Bruel <Christian.Bruel@st.com>
cc: egcs-patches@cygnus.com
Subject: Re: dead code elimination after constant propagation 
Reply-To: law@cygnus.com
In-reply-to: Your message of Tue, 05 Jan 1999 15:02:13 MST.
             <199901051403.PAA17675@eux100.sgp.st.com> 
Date: Wed, 06 Jan 1999 01:24:46 -0700
Message-ID: <3422.915611086@hurl.cygnus.com>
From: Jeffrey A Law <law@hurl.cygnus.com>


  In message <199901051403.PAA17675@eux100.sgp.st.com>you write:
  > The problem comes from flow.c : The function "propagate_block" scans
  > backward the insns for each basic block, recording in last_mem_set
  > a set mem RTL (candidate for removing).
  > However, if several RTLs are dead in the same time, only the last one
  > will match with the function insn_dead_p, because "last_mem_set"
  > containts just one element.
  > 
  > The following patch is a quick fix to this problem, it can be improved,
  > but showed some improvements on several non regression benchs.
  > 
  > Thanx for your comments,
Thanks for the patch.  Several comments.

First, always include a ChangeLog entry with a patch.  The ChangeLog entry
should be plaintext (not a diff).  It should describe the changes to each
file & function.  See gcc/ChangeLog for examples.

Your patch was reversed -- Use diff <args> old new.

Please include "-p"  in your diff args.  It helps us read the patch.  Something
like this is common:

diff -c3p old new

or

diff -up old new


  > -#define MAX_DEAD_CONST_PROPAGATE_CHECK 1000
  > -static rtx *last_mem_set;
  > -static int n_mem_set;
  > -static int max_mem_set = MAX_DEAD_CONST_PROPAGATE_CHECK;
  > +static rtx last_mem_set;
I'd recommend using virtual arrays or malloc/xrealloc to dynamically size the
last_mem_set array.  That avoids unnecessary constraints on the size of the
array and 99% of the time it'll use less memory than statically capping it
at 1000 entries.


  > @@ -1900,12 +1893,9 @@
  >  	return ! cc0_live;
  >  #endif
  >        
  > -      if (GET_CODE (r) == MEM && ! MEM_VOLATILE_P (r)) {
  > -	int i;
  > -	for(i = 0; i < n_mem_set; i++)
  > -	  if(rtx_equal_p (r, last_mem_set[i]))
  > -	    return 1;
  > -      }
  > +      if (GET_CODE (r) == MEM && last_mem_set && ! MEM_VOLATILE_P (r)
  > +	  && rtx_equal_p (r, last_mem_set))
  > +	return 1;
You need to follow GNU coding conventions in this code.

"{" and "}" belong on lines by themselves with a 2 space indention from the
previous line.

All lines inside the {} should be indented 2 more spaces from the {}

Always put a space before an open paren (the "for" and "if" statements above
are missing spaces are are other parts of your patch).


  >  
  >        while (GET_CODE (r) == SUBREG || GET_CODE (r) == STRICT_LOW_PART
  >  	     || GET_CODE (r) == ZERO_EXTRACT)
  > @@ -2118,7 +2108,6 @@
  >  {
  >    register int regno;
  >    register rtx reg = SET_DEST (x);
  > -  int i;
  >  
  >    /* Modifying just one hardware register of a multi-reg value
  >       or just a byte field of a register
  > @@ -2141,27 +2130,17 @@
  >       address of the last thing stored into memory, show we don't know
  >       what the last store was.  If we are writing memory, save the address
  >       unless it is volatile.  */
  > -  for(i = 0; i < n_mem_set; i++)
  > -    if (last_mem_set[i] &&
  > -	((GET_CODE (reg) == REG && reg_overlap_mentioned_p (reg, last_mem_set[i
  > ])) ||
  > -	 (GET_CODE (reg) == MEM && rtx_equal_p (reg, last_mem_set[i]))))
  > -      last_mem_set[i] = 0;
  > +  if (GET_CODE (reg) == MEM
  > +      || (GET_CODE (reg) == REG
  > +	  && last_mem_set != 0 && reg_overlap_mentioned_p (reg, last_mem_set)))
  > +    last_mem_set = 0;
This code has similar GNU coding convention problems as the code I mentioned
above.  You'll need to fix them.

Also note that when you continue lines you should do so like this

  if (foo
      && bar
      && (com || oof))

Do not write lines with more than 80 characters.  Wrap them as I've shown
above.

Your handling of MEMs here is not correct.

You need to invalidate all MEMs that might refer to the same memory location,
not just those which are "rtx_equal_p" to the one currently being set.

You'll probably need to add a call to "init_alias_analysis" somewhere in the
flow code to initialize the alias analysis tables. Then you'll want something
like this to determine if two MEMs can access the same memory location.

	if (GET_CODE (reg) == MEM
	    && output_dependence (reg, last_mem_set[i])
	  last_mem_set[i] = NULL_RTX;


 >      
  >    if (GET_CODE (reg) == MEM && ! side_effects_p (reg)
  >        /* There are no REG_INC notes for SP, so we can't assume we'll see 
  >  	 everything that invalidates it.  To be safe, don't eliminate any
  >  	 stores though SP; none of them should be redundant anyway.  */
  > -      && ! reg_mentioned_p (stack_pointer_rtx, reg)) {
  > -    for(i = 0; i < n_mem_set; i++)
  > -      if(last_mem_set[i] == 0) {
  > -	last_mem_set[i] = reg;
  > -	goto cont;
  > -      }
  > -    if(n_mem_set < max_mem_set)
  > -      last_mem_set[n_mem_set++] = reg;
  > -  }
  > -
  > - cont:
  > +      && ! reg_mentioned_p (stack_pointer_rtx, reg))
  > +    last_mem_set = reg;
Don't use a goto here.  This code can be easily rewritten to avoid the goto.
You also need to fix several GNU coding standards problems here.  The same
problems I've mentioned earlier.

  > @@ -2564,13 +2543,8 @@
  >        if (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
  >  	  && CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))
  >  	; /* needn't clear last_mem_set */
  > -      else {
  > -	int i;
  > -	for(i = 0; i < n_mem_set; i++)
  > -	  if(rtx_equal_p (x, last_mem_set[i])) {
  > -	    last_mem_set[i] = 0;
  > -	  }
  > -      }
  > +      else
  > +	last_mem_set = 0;
The usual comments -- coding standards violatons.

This code is also wrong.  When you read from a memory location you have 
to invalidate all the stores which *might* reference the same memory.

   for (i = 0; i < n_mem_set; i++)
     if (true_dependence (last_mem_set[i], GET_MODE (x),
			  x, rtx_addr_varies_p))

Or something like that.


jeff

------- End of Forwarded Message



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