Re: [4.5] Autoinc support in tree-ssa-loop-ivopts.c

[Bernd Schmidt <bernds_cb1 at t-online dot de>]

Zdenek Dvorak wrote:
> Hi,
>
> > > Another thing: is this really the right condition to detect?  I would
> > > expect autoinc modes to be useful even in cases where more than one
> > > memory reference and possibly also the exit test are based on the
> > > autoincremented induction variable. 
> > Here's a new patch which has been extensively changed.
> >
> > One possible condition to test for would be whether the uses closest to 
> > the position of the biv increment (either before or after) allow 
> > autoincrement.  That seems to me rather expensive since we'd have to 
> > walk the basic blocks a few times to determine an order for the uses, so 
> > I've chosen something slightly weaker: either all uses before the 
> > increment, or all uses after the increment must allow autoincrement. 
> > This condition gives us a strict superset of opportunites compared to 
> > the one used by the previous patch.
> >
> > The cost of the code in iv_ca_recount_cost is reduced by this since most 
> > of the tracking is now done in iv_ca_set{_no,}_cp.
> >
> > I've added code to test for the availability of each of the four autoinc 
> > addressing modes for every machine mode.
> >
> > Not too much testing yet other than comparing generated code for a lot 
> > of examples.  If this looks OK to you, I'll start bootstrap etc.
>
> yes, this looks fine.
>
> > +  autoinc = false;
> > +  msize = GET_MODE_SIZE (mem_mode);
> > +  autoinc_offset = offset;
> > +  if (stmt_after_inc)
> > +    autoinc_offset += cstep * ratio; 
> > +  if (s_offset || symbol_present || var_present || ratio != 1)
> > +    autoinc = false;
> > +  else if (0
> > +	   || (has_postinc[mem_mode] && autoinc_offset == 0
> > +	       && msize == cstep)
> > +	   || (has_postdec[mem_mode] && autoinc_offset == 0
> > +	       && msize == -cstep)
> > +	   || (has_preinc[mem_mode] && autoinc_offset == msize
> > +	       && msize == cstep)
> > +	   || (has_predec[mem_mode] && autoinc_offset == -msize
> > +	       && msize == -cstep))
> > +    autoinc = true;
>
> This is rather hard to read; as far as I can tell, your code is
> equivalent to
>
> autoinc = false;
> if (s_offset == 0 && !symbol_present && !var_present && ratio == 1)
>   {
>     msize = GET_MODE_SIZE (mem_mode);
>     if ((has_postinc[mem_mode] && !stmt_after_inc && msize == cstep)
>         || (has_postdec[mem_mode] && !stmt_after_inc && msize == -cstep)
>         || (has_preinc[mem_mode] && stmt_after_inc && msize == cstep)
>         || (has_predec[mem_mode] && stmt_after_inc && msize == -cstep))
>        autoinc = true;
>   }

If it is, then that's unintentional.  I think the first test for 
s_offset shouldn't be in there, it was part of one of those late changes 
before submission...
An example - for postinc, an address before the increment should have 
offset 0, while an address after the increment should have offset -4.

Also, the if (0) part has to go, that's from when I was still using 
#ifdef HAVE_...CREMENT blocks.

> This loop can be avoided, by doing the computation in iv_ca_set_no_cp
> and iv_ca_set_cp;  [...]

However, I find the code as it is now more easily understandable and 
modifiable than you suggestion.  Do you believe it'll make a measurable 
change?


Bernd
--
This footer brought to you by insane German lawmakers.
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft Muenchen, Registergericht Muenchen HRB 40368
Geschaeftsfuehrer Thomas Wessel, William A. Martin, Margaret Seif