Re: Reload patch v4

[Bernd Schmidt <crux at pool dot informatik dot rwth-aachen dot de>]

Short note at the beginning: I'll cut some of your comments; those that are
obvious mistakes which I'll fix.

>   Can find_clobbered_regs be called with SUBREG (hard reg)?  If it
>   does, then you need to handle it.

Indeed, that looks like a bug.

>   Do we still need reg_where_born?  Any other arrays that need to be
>   eliminated?

Looking at the patch now, I'd say that it could be replaced by a macro
#define REG_WHERE_BORN(x) \
  (reg_where_born_exact[x] - reg_where_born_clobber[x])

I'll try this.

>   In insert_save_restore, don't you need to set the mode and code for
>   insns created by this routine?  The old code used to do this, but
>   I do not see that your code does this.  Actually I can see that
>   you *might* not need to set the mode right now, but I'm not 100%
>   sure.

It seems that I accidentally deleted the line that sets INSN_CODE.  Good
that you spotted this.  However, the mode is no longer relevant.  The
old code used to encode whether an insn needs reloading and/or eliminations
in the machine mode; the new code has special fields need_reload and
need_elim in struct insn_chain.  But now I think I see another bug: if
the new insn needs elimination, it isn't placed on the insns_need_elim
chain by caller-save.c.
 
> local-alloc.c:
> 
>   I understand your desire to remove the scratch handling in local
>   alloc.  But I'm not sure it's wise at this point.
> 
>   In general, the more things we can allocate in local alloc the
>   better -- our algorithms for local register are much better than
>   those for global register allocation.

Yes, but why allocate scratches?  They live only during one insn, so any
register that matches should be OK - and reload can allocate this register
as well as local-alloc, unless I am missing something.  In fact, local-alloc
might even choose a register of the wrong class, or allocate a register
where none is in fact needed, since it can only guess which of the
alternatives will be chosen by reload.
Previously, the code might have served the purpose to try to avoid spilling.
If local-alloc managed to get a register of the right class, reload wouldn't
have had to allocate a spill register.  Now that it's much cheaper to spill
a register, I'm not sure the code really needs to be there.

>   In theory, we should be able to revisit this after dealing with
>   the rest of your reload changes, right?

That's correct.  If it turns out that the code was in fact important, it
should be possible to re-add it.
 
>   Also note that whether or not we keep the local-alloc.c code
>   determines what we do with related hunks of code in global.c
>   and reload1.c.

Obviously, I deleted those as well for now.

>   In reg_becomes_live I see a "Subreg here" warning.  I do think
>   that warning can trigger.

It does trigger.  I've seen it a couple of times, and the fact that it
is left in is merely an oversight.
 
>   In build_insn_chain, don't you have to be prepared to handle a
>   REG_DEAD note for a SUBREG?  Or is there some reason this can
>   not happen?

flow.c only appears to be making REG_DEAD notes for full registers.  I
don't know whether SUBREGs can appear in other parts of the compiler.
I just had a look at sched.c, it has a bit of code that does

if ((REG_NOTE_KIND (link) == REG_DEAD
     || REG_NOTE_KIND (link) == REG_UNUSED)
  /* Verify that the REG_NOTE has a valid value.  */
    && GET_CODE (XEXP (link, 0)) == REG)

It does not handle a SUBREG either.  When I wrote the code, I probably
followed examples like this.

> reload.h:
> 
>   Why is struct insn_chain's definition dependent on SET_HARD_REG_BIT?

If I remember correctly, it's because some files include reload.h, but
not hard-reg-set.h, and that leads to compilation errors.

> reload1.c (the biggie):
> 
>      available hard regs can be found.  Spilling can invalidate more
>      insns, requiring additional need for reloads, so we must keep checking
>      until the process stabilizes.
> +    If we have register life information, it is often possible to avoid
> +    spilling all the pseudos allocated to a reload register.
> I would actually reword this comment.

Yes.  It is left over from patch v1 which worked rather differently than
the latest one.
 
> Aren't fixed registers and eliminable registers still in "bad_spill_regs"?
> The current comment does not list them explicitly (I think it should 
> mention them explicitly).

Eliminables aren't, at the moment, see below.  Fixed registers are in fact
in bad_spill_regs; the comment needs to be fixed.

> I think you should delete all the code related to "used_spill_regs"
> since we're deleting the only user of this information in reorg.c

There's one user still in finish_spills, but I'm sure that can be removed.

> What's the difference between reload_firstobj and reload_startobj?

reload_startobj marks the beginning of the reload_obstack.  All of the
insn_chain structures are allocated before reload runs, and freed at
the end by
  obstack_free (&reload_obstack, reload_startobj);
reload_firstobj is used to free memory that is allocated while processing
a single instruction.

> Can you explain the problems with caller-save and the new reload code
> in a little more detail?

Normal caller-saves aren't a problem, they should work fine even with the
new code.  The problem is with caller-saves that require a spill register.
In save_call_clobbered_regs, an insn that restores a caller-saved register
can be inserted before any other instruction.  This means we don't know
exactly where we are going to need this spill register.  For the previous
code, that didn't matter, since it could allocte a spill register globally.
With the new code, a spill register is freed only for a given instruction,
and we don't know beforehand where it's going to be needed.

> +   /* Let's try without this, I can't see a reason for doing this.  */
> + #if defined ELIMINABLE_REGS && 0
> +   /* If registers other than the frame pointer are eliminable, mark them as
> +      poor choices.  */
> +   for (i = 0; i < NUM_ELIMINABLE_REGS; i++)
> +     SET_HARD_REG_BIT (bad_spill_regs, reg_eliminate[i].from);
> +
> + #endif
> 
> You should reenable this code.  Imagine a machine which uses an arg
> pointer ;-)  You really don't want to spill it if at all possible.
> Similarly if the machine has other eliminable regs.

Why not, if it is going to be eliminated?  The whole idea behind register
elimination is to make more registers available to be allocated for other
purposes.  At the point where the above code is run, reload thinks that
it's going to eliminate the register, so it might as well be used for spills.
Once it is eliminated, it behaves just like any other hard-register - or
doesn't it?

> I think we need to fix forget_old_reloads_1 to handle cases where we
> store into a pseudo which happens to be allocated to a hard reg that
> has also been used as a spill reg.  Imagine
> 
> 	Use reg 0 as an output reload for some value V
> 
> 	Psuedo X is made live and is allocated to reg 0
> 	[ ... ]
> 	Pseudo X dies
> 
> 	Use of V which needs an input reload.
> 
> In this case we might try and use reg0 to satisfy the input reload
> even though it no longer holds the value we need.

I need to look at this more carefully later, I can't comment on it right
now.

Again, thanks for the feedback.  I'll look into the issues you brought up,
and I'll start testing on some Sparc machines (and possibly HPPAs as well).

Bernd