Re: [PATCH, PR target/65103, 2/3] Propagate address constants into loops for i386

[Jeff Law <law at redhat dot com>]

On 04/17/2015 02:34 AM, Ilya Enkovich wrote:
> On 15 Apr 14:07, Ilya Enkovich wrote:
> > 2015-04-14 8:22 GMT+03:00 Jeff Law <law@redhat.com>:
> > > On 03/15/2015 02:30 PM, Richard Sandiford wrote:
> > > > Ilya Enkovich <enkovich.gnu@gmail.com> writes:
> > > > > This patch allows propagation of loop invariants for i386 if propagated
> > > > > value is a constant to be used in address operand.  Bootstrapped and
> > > > > tested on x86_64-unknown-linux-gnu.  OK for trunk or stage 1?
> > > >
> > > >
> > > > Is it necessary for this to be a target hook?  The concept doesn't seem
> > > > particularly target-specific.  We should only propagate into the address
> > > > if the new cost is no greater than the old cost, but if the address
> > > > meets that condition and if propagating at this point in the pipeline is
> > > > a win on x86, then wouldn't it be a win for other targets too?
> > >
> > > I agree with Richard here.  I can't see a strong reason why this should be a
> > > target hook.
> > >
> > > Perhaps part of the issue here is the address costing metrics may not have
> > > enough context to make good decisions.  In which case what context do they
> > > need?
> >
> > At this point I don't insist on a target hook.  The main reasoning was
> > to not affect other targets. If we extend propagation for non constant
> > values different aspects may appear. E.g. possible register pressure
> > changes may significantly affect ia32. I just wanted to have an
> > instrument to play with a propagation on x86 not affecting other
> > targets. I don't have an opportunity to test possible performance
> > implications on non-x86 targets. Don't expect (significant)
> > regressions there but who knows...
> >
> > I'll remove the hook from this patch. Will probably introduce it later
> > if some target specific cases are found.
> >
> > Thanks,
> > Ilya
> >
> > > Jeff
>
> Here is a version with no hook.  Bootstrapped and tested on x86_64-unknown-linux-gnu.  Is it OK for trunk?
>
> Thanks,
> Ilya
> --
> gcc/
>
> 2015-04-17  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	PR target/65103
> 	* fwprop.c (forward_propagate_into): Propagate loop
> 	invariants if a target says so.
>
> gcc/testsuite/
>
> 2015-04-17  Ilya Enkovich  <ilya.enkovich@intel.com>
>
> 	PR target/65103
> 	* gcc.target/i386/pr65103-2.c: New.
It seems to me there's a key piece missing here -- metrics.

When is this profitable, when is it not profitable.   Just blindly 
undoing LICM seems wrong here.

The first thought is to look at register pressure through the loop.  I 
thought we had some infrastructure for this kind of query available. 
It'd probably be wise to re-use it.  In fact, one might reasonably ask 
if LICM should have hoisted the expression to start with.


I'd also think the cost of the constant may come into play here.  A 
really cheap constant probably should not have been hoisted by LICM to 
start with -- but the code may have been written in such a way that some 
low cost constants are pulled out as loop invariants at the source 
level.  So this isn't strictly an issue of un-doing bad LICM

So I think to go forward we need to be working on solving the "when is 
this a profitable transformation to make".

jeff