Re: [PATCH] Small cleanups

[Richard Guenther <rguenther at suse dot de>]

On Sun, 15 Mar 2009, Ulrich Weigand wrote:

> Richard Guenther wrote:
> > On Thu, 12 Mar 2009, Ulrich Weigand wrote:
> > > Before this patch, the PARAM_MAX_COMPLETELY_PEELED_INSNS parameter
> > > was always considered to limit the number of insns *added* by loop
> > > unrolling, as I understand (and this still seems to be the case in
> > > loop-unroll.c).  Your new check, however, compares the *total* number
> > > of instructions after loop unrolling (including the initial copy)
> > > against the parameter.
> > > 
> > > Was this a deliberate change?  I'm wondering if the check shouldn't
> > > rather be something along the lines of
> > > 
> > >        if (unr_insns > ninsns
> > >  	  && (unr_insns - ninsns
> > >  	      > (unsigned) PARAM_VALUE (PARAM_MAX_COMPLETELY_PEELED_INSNS)))
> > 
> > It wasn't a deliberate change.  Does it cause any problems for you?
> 
> Not problems as such, just trying to understand what's going on ...
> 
> The core problem is that in some cases, complete loop unrolling causes
> unacceptable code size growth on the SPU (Trevor has some test cases).
> That's why the SPU back-end today sets PARAM_MAX_COMPLETELY_PEEL_TIMES
> to 1, which basically disables this pass.
> 
> On mainline, this now causes a number of testcases to fail.  In looking
> at these, it appeared to me that at least in *those* cases, complete
> unrolling *would* be beneficial even on the SPU, because it actually
> *decreases* total code size anyway.
> 
> So I was investigating whether there is a better way of tuning those
> parameters than what we currently do, and thus I noticed that the
> semantics of the PARAM_MAX_COMPLETELY_PEELED_INSNS apparently isn't
> completely clear ...
> 
> However, on looking closer, that doesn't really matter for our cases,
> because for default operation at -O2 (with no -funroll-loops etc. flag),
> that parameter isn't even looked at -- the intention of the code is
> clearly to *never* unroll if total code size increases.
> 
> I guess the question now becomes why this apparently doesn't work with
> Trevor's original test cases.  Maybe the estimated_unrolled_size  
> heuristics aren't appropriate?

Well, they are what they are called.  An estimation.  An estimation
that tries to account for possible code savings by removing induction
variable updates and other "general" optimizations.  Joern has
posted some patches that would account for i-cache size of a possible
other loop.

Richard.