Re: hard register reload patch

[Joern Rennecke <amylaar at cygnus dot co dot uk>]

> Some comments at the top which aren't directly related to this patch.
> I believe the central problem we need to solve before considering the parts
> of this patch related to hard register reloading is how to treat hard
> register references on ports that define SMALL_REGISTER_CLASSES.  Currently,
> this issue is very muddy.
> 
> I suggest we keep the rules simple and easy to understand: the only insns
> which reference hard registers may be moves which copy them into/out of
> pseudos.

This is muddy, too.  What is a copy?  On some hardware, signe and/or zero
extension is part of a move insn.

And yet this rule is too strict because it would disallow using a hard
register where one is naturally used - where you have and insn that
can take only one particular register as one of its inputs.  If you accept
a pseudo in this place, cse/gcse/loop/combine get taken away and create
code that would need dozens of these one-of-a-kind registers to be live
simulatanously to avoid lots of reloads.

>           Hard registers may not live across any other insns.

They have to - otherwise you can't have function calls which take more than
one parameter in registers.

> To me, this appears to be what was originally intended, but it's not how all
> parts of the compiler behave these days.  [I have an unreviewed combiner
> patch (~ 10 months old) which tries to replace the (convoluted) logic dealing
> with hard regs and SMALL_REGISTER_CLASSES with code that just enforces these
> rules.  This patch fixes two compiler aborts when compiling the Linux kernel
> with -mregparm on the i386.]

Are these aborts becasue the reload runs out of spill registers?  I suppose
by patch could fix these problems, too.

> The SH port explicitly sets this up in some patterns.  (Some of the following
> is partly guesswork, since I haven't yet seen the testcase that led to this
> patch).  Take a pattern like this:
> 
> (define_insn "fix_truncdfsi2_i"
>   [(set (reg:SI 22)
>         (fix:SI (match_operand:DF 0 "arith_reg_operand" "f")))
>    (use (match_operand:PSI 1 "fpscr_operand" "c"))]
>    "TARGET_SH4"
>    "ftrc %0,fpul"
>   [(set_attr "type" "dfp_conv")])
> 
> If I recall Joerns explanation correctly, the problem is that the input
> operand may need reg 22 (fpul) for a secondary reload.

Yes.

>                                                         IMO, the pattern
> should be changed to use a match_operand with a single register class for
> the output operand.  That should enable reload to work correctly; we'd get
> two reloads for reg 22 which don't conflict.
> Joern believes that this approach would cause unacceptable loss of code
> quality, which is possible, but I haven't seen any evidence yet to support
> this.
> 
> There was a discussion about this a month or two ago (before someone tries to
> find it in the mail archives: that was on the cygnus local gcc list).  I sent
> a patch against sh.md plus some analysis in which I tried to show that code
> quality is the same on average.  (Note that my patch wasn't tested for
> correctness; as I said I'm lacking a testcase.  It may have been completely
> bogus).

But this is ignoring the fact that the current shunning of explicitly
mentioned registers is pessimizing patterns that use them.  And my patch
aims to fix that.
Moreover, I am not certain that the code you used is sufficiently
representative of performance and size critical code on the SH to
make the meansurements meaningful.

> >   > reg_rtx field set, and it is equal to the in or out field, or to both.
> > ?!? Can't we tell by the reload type?  Or maybe it would be better to have a
> > flag which indicates that this is a hard reload.

I though about that, but it doesn't seem to make sense from a simplicity
standpoint.  Creating lots of new reload types would make reload even
more complex.  An extra flag doesn't really buy much, since you still
want to know if it's an input, output or input-output hard reload.

> Absolutely.  IMO, relying on special combinations of values is brittle and
> non-obvious.  That reminds me, we have some code in reload at the moment
> which does something similar:
> 
>    /* AUTO_INC reloads need to be handled even if inherited.  We got an
>       AUTO_INC reload if reload_out is set but reload_out_reg isn't.  */
> 
> This kind of thing is just horrible, IMO.  It's made worse by the fact that
> reload_out_reg isn't documented and I have a feeling that the documentation
> for reload_in_reg isn't totally accurate either.  Joern, if you fully
> understand the way these two fields are used, it would be great if you could
> add some documentation.

For this case, an extra flag would indeed make sense - that's easier to
test and easier to read.  It wasn't an option at the time the code was
written, becasue we had separate arrays all over the place, but now that
we have the struct reload definition, a one-bit-flag is cheap to add.

> > I sincerely hope you documented this in the code.  One of my biggest issues
> > with the reload pass is the precise lifetimes of reloads are not documented.
> > I'd hate to see us continue that terrible habit.
> 
> I'd like to see all the RELOAD_FOR_xxx codes go away.  I've got half-done
> patches to scan the rtl for replacements and thus build a definition/use
> chain for reloads which should enable us to schedule them precisely with
> much simpler code than reload_reg_free_xxx/reloads_conflict/etc.  But I'll
> leave that discussion for another occasion 8)

How about this: send me a copy now.  If I think it makes sense, I'll see
how it'll integrate with my patch - then we need not worry to document stuff
that is going away anyways.  If not, we can discuss it separately.

> >   > instead of using regsets live_before and live_after, which were used
> >   > in tandem all the time, I use live_throughout and dead_or_set.
...
> Right.  I haven't really tried understanding the patch yet because too many
> parts of it seem to do something unrelated - e.g. I don't see how hard
> register reloading could influence caller-saves, so I'm not sure what those
> parts of the patch are doing.

Well, it's part of the live_before/live_after -> live_throughout/dead_or_set
conversion.
And that change is needed as a prerequisite for the hard reloads.