Re: [4.5] Doloop improvement patches, 2/7

[Zdenek Dvorak <rakdver at kam dot mff dot cuni dot cz>]

Hi,

> >> @@ -1853,6 +1853,43 @@ simplify_using_initial_values (struct lo
> >>  		  SET_REGNO_REG_SET (this_altered, i);
> >>  	    }
> >>  
> >> +	  did_replace = simplify_using_assignment (insn, expr);
> >> +
> >> +	  if (did_replace)
> >> +	    {
> >> +	      rtx *pnote, *pnote_next;
> >> +	      for (pnote = &cond_list; *pnote; pnote = pnote_next)
> >> +		{
> >> +		  rtx note = *pnote;
> >> +
> >> +		  pnote_next = &XEXP (note, 1);
> >> +		  simplify_using_assignment (insn, &XEXP (note, 0));
> >> +		  /* We can no longer use a condition that has been simplified
> >> +		     to a constant, and simplify_using_condition will abort if
> >> +		     we try.  */
> >> +		  if (CONSTANT_P (XEXP (note, 0)))
> >> +		    {
> >> +		      *pnote = *pnote_next;
> >> +		      pnote_next = pnote;
> >> +		      free_EXPR_LIST_node (note);
> >> +		    }
> >> +		}
> >> +	    }
> > 
> > The block above should not be guarded by did_replace.   Also, it
> > seems that you should remove the conditions that are invalidated
> > according to this_altered from the list, unless you perform a
> > replacement in the condition.
> 
> >> +	  if (old != *expr)
> > 
> > Why is this guarded by old != *expr, and not did_replace?
> 
> It seems that maybe did_replace was not a good choice of name for this.
>  It is set to true if we called simplify_replace_rtx on the expression,
> but that does not necessarily mean that a replacement actually took
> place.  It only means that the insn is of a form that we can understand.
> 
> So, guarding the first block with did_replace is useful because
> otherwise we would just do unnecessary work (we'd call
> simplify_using_assignment several times, and it would fail each time).
> 
> The second block is guarded by "old != *expr" to make sure we actually
> have a new version of the expression.  Otherwise, again, we'd do
> unnecessary work.

hmm... that certainly is rather counterintuitive.  I'd recommend to just
drop did_replace, the performance gain is probably negligible compared
to the maintentance nightmare potential :-)

What about removing the conditions invalidated by this_altered?
I guess it is not actually needed now because simplify_using_condition
tests the set of altered registers, but it might be nicer to
deal with the registers only in simplify_using_initial_values (similarly
like you did for simplify_using_assignment),

Zdenek