Re: [PATCH]: PR 11271 Handle reloading of R+R+CONST for any reg

[Ulrich Weigand <weigand at i1 dot informatik dot uni-erlangen dot de>]

Eric Botcazou wrote:

> [Sorry for the delay, I was travelling lately too :-)]

Same here, I was away this week ...

> > I think there are really two separate issues.  The one is a question of
> > optimization: is splitting out the REG + CONST into a separate reload
> > beneficial on the particular platform?  On s390, this is likely not to
> > be true in the case of (non-base REG + REG + CONST), so I don't want
> > that case to hit in any case.
> 
> Do you mean that the 3-part address pattern is valid on s390? If so, it's 
> indeed partly an optimization issue on that platform. But on SPARC (and on 
> arm according to your remark below) it's really only a correctness issue, 
> because it's the only place where (REG + REG + CONST_INT) can be untangled 
> into the canonical (REG + REG). If it doesn't match here, the compiler ICEs.

To clarify: yes, the REG + REG + CONST is valid on s390, provided both REGs
are base registers and the CONST is in range.  This is the optimization
issue I see here: after the current changes reload now unnecessarily avoids
making full use of this address format on s390 in some cases.

> > The other issue is a correctness issue: if find_reloads_address decides
> > to perform the split, everything should continue to work.  I've looked
> > a bit more into why it currently breaks on s390.
> > [...]
> > Now, what looks broken to me in this scenario is that I thought all
> > changes done by find_reloads should be reverted in the
> > calculate_needs_all_insns loop, so that each pass through the loop finds
> > the original insn again.
> 
> <naive question>
> How would the loop terminate if the same reloads were scheduled for each
> iteration?
> </naive question>

One pass through calculate_needs_all_insns may fail if it turns out that
more reload registers are required within a single insn than there are
spill registers currently available.  Then, another register is allocated
as spill register (causing register allocation to be re-done), and we
make another full pass through calculate_needs_all_insns.  This now
needs to start completely from scratch, taking the changed situation
into account.  Thus all changes made prematurely in find_reload need
to be canceled (and this is indeed usually the case, except that the
recent change broke it).

Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  weigand@informatik.uni-erlangen.de