This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: What to do with new-ra for GCC 4.0


Bernd Schmidt wrote:

> I'm attaching a proof-of-concept patch, against a checkout from 
> 20050106.  The main things this patch does
>   * replace reload insn ordering using RELOAD_FOR_blah by dependencies
>     automatically generated from the replacements
>   * replace reload inheritance by a separate pass that is run on data
>     collected by find_reloads
>   * try to make inheritance powerful enough that many other random
>     optimizations scattered across reload can be deleted (I hope that
>     eventually, reload_cse_regs can go as well)

This is very nice!  I've tried the patch on s390; here's what I found
until now ...

> The changes in genoutput.c are necessary to deal with the fact that some 
> machine descriptions have output operands without a "=" constraint letter.

Wouldn't it be better to simply fix the backends?  Testing your patch
revealed a bug in s390.md where I had a superfluous "=" ...

> It compiles (tested on i686-linux) and holds up pretty well for the 
> testsuite, but it does not bootstrap yet.

I've needed the following changes to build the C compiler and run its
test suite without regressions on s390; I haven't attempted bootstrap or
the other languages yet.

(I must say I found your code *much* easier to debug than the old
reload inheritance code ;-))


>! compute_birth_death (struct insn_chain *chain)
>  {
>
>!   /* Second pass over the secondary reloads, this time to make sure secondary
>!      reloads and their primaries conflict.  */
>!   for (i = 0; i < chain->n_reloads; i++)
>!     {
>!       struct reload *rl = chain->rld + i;
>!       if (rl->secondary_in_reload >= 0)
>! 	rl->reginfo.birth--;
>
>!       if (rl->secondary_out_reload >= 0)
>! 	{
>! 	  struct reload *rl2 = chain->rld + rl->secondary_out_reload;
>! 	  rl2->reginfo.birth--;
>! 	}
>!     }
This is wrong (at least, it is different from what the old code did
and what s390 expects).

For secondary input reloads, you decrement 'birth' of the primary
reload.  This prevents the same reload reg to be chosen for primary
and secondary reload.  However, the old code *did* do that (and
existing backends expect it) *if* primary and secondary reload
share the same class.  On the other hand, if some hard register
feeds into the primary reload and dies there, because 'birth'
of the secondary reload is equal to the original birth of the
primary reload, you now accept such a register as secondary
reload reg.  That's broken because the secondary reload reg is 
a scratch register needed to implement the load, which may use
registers feeding into the primary.

For example, on s390 we need a secondary reload for
  (plus:SI (reg:SI 1) (const_int 10000))
This gets transformed into
  (set (reg:SI scratch) (const_int 10000))
  (set (reg:SI target) (plus:SI (reg:SI 1) (reg:SI scratch)))
where target is the primary reload register and scratch is
the secondary reload reg.  If reg:SI 1 dies here, it is crucial
that reg 1 is *not* used as secondary reload reg.  (Of course
using it as primary reload reg is just fine!) 

So I think instead of rl->reginfo.birth-- we need

         struct reload *rl2 = chain->rld + rl->secondary_in_reload;
         rl2->reginfo.birth--;

(I haven't thought about the secondary output situation yet.)


>! init_rlinsns (struct insn_chain *chain)
>! {
>
>!   REG_SET_TO_HARD_REG_SET (chain->hard_live_across, &chain->live_before);
>!   REG_SET_TO_HARD_REG_SET (tmp, &chain->live_before);
I guess this should be live_after, not live_before.
>!   AND_HARD_REG_SET (chain->hard_live_across, tmp);


>! calculate_needs_all_insns (int global)
>  {
>! 	  if (num_eliminable || num_eliminable_invariants)
>! 	    did_elimination = eliminate_regs_in_insn (copy, 0);
>  
>! 	  if (GET_MODE (insn) == SImode)
Should be copy, not insn.


>! usable_for_inheritance (rtx head_rtx, struct reload *rl, rtx this_rtx,
>! 			int regno, HARD_REG_SET *usable_regs,
>! 			HARD_REG_SET *unallocated)
>! {
>!   enum machine_mode mode = rl->mode;
>!   int can_use_inheritance_reg = rl->out_reg == 0;
>!   int nregs, k;
>!   int offsetted_regno = regno;
>! 
>!   if (GET_CODE (head_rtx) == REG && GET_CODE (this_rtx) == SUBREG)
>!     offsetted_regno += subreg_regno_offset (regno,
>! 					    GET_MODE (SUBREG_REG (this_rtx)),
>! 					    SUBREG_BYTE (this_rtx),
>! 					    GET_MODE (this_rtx));
>! 
>!   if (GET_MODE (head_rtx) != mode)
>!     {
>! #ifdef CLASS_CANNOT_CHANGE_MODE
>!       if (TEST_HARD_REG_BIT (reg_class_contents[CLASS_CANNOT_CHANGE_MODE],
>! 			     regno)
>! 	  && CLASS_CANNOT_CHANGE_MODE_P (regno, mode))
>! 	return IT_NONE;
>! #endif
>      }
>  
>!   if (! HARD_REGNO_MODE_OK (offsetted_regno, mode))
>!     return IT_NONE;
>! 
>!   nregs = HARD_REGNO_NREGS (offsetted_regno, mode);
>! 
>!   for (k = 0; k < nregs; k++)
>      {
>!       if (! TEST_HARD_REG_BIT (*usable_regs, offsetted_regno + k))
>! 	return IT_NONE;
>!       if (! TEST_HARD_REG_BIT (*unallocated, offsetted_regno + k))
>! 	can_use_inheritance_reg = 0;
>      }
>+ 
>+   /* Can we inherit for this opportunity?  */
>+   if (! can_use_inheritance_reg
>+       && ! suitable_for_copy (offsetted_regno, mode, rl->class))
>+     return IT_NONE;
>+ 
>+   return can_use_inheritance_reg ? IT_USE_REG : IT_OVERRIDE_INPUT;
>  }
This is probably wrong for mixed-mode reloads (rl->mode != rl->inmode,
e.g. for some multiply insns on s390 that have a DImode output register
that is matched with a SImode input).  In those cases we cannot use the
inherited value as reload register; and in testing whether it is usable
as copy, we only need to test HARD_REGNO_NREGS (... rl->inmode) many
registers.


>! perform_inheritance (rtx head_rtx, struct inherit_chain *this,
>! 		     struct reload *this_rl, int best_reg,
>! 		     HARD_REG_SET *usable_regs)
>! {
>!   rtx this_rtx = (this->rli->type == RLI_OUTPUTRELOAD
>! 		  ? this_rl->out_reg : this_rl->in_reg);
>!   enum machine_mode mode = this_rl->mode;
>!   enum inherit_type itype;
>!   HARD_REG_SET this_usable;
>!   int offsetted_regno = best_reg;
>! 
>!   if (GET_CODE (head_rtx) == REG && GET_CODE (this_rtx) == SUBREG)
>!     offsetted_regno += subreg_regno_offset (best_reg,
>! 					    GET_MODE (SUBREG_REG (this_rtx)),
>! 					    SUBREG_BYTE (this_rtx),
>! 					    GET_MODE (this_rtx));
>! 
>!   compute_unallocated (this->chain, this_rl, &this_usable);
>! 
>!   itype = usable_for_inheritance (head_rtx, this_rl, this_rtx, best_reg,
>! 				  usable_regs, &this_usable);
>!   if (itype == IT_NONE)
>!     return 0;
>  
>!   if (this_rl->optional && ! this->was_enabled)
>!     find_optional_reg (this_rl, &this_usable, offsetted_regno, 1);
>!   else if (itype == IT_USE_REG)
>!     {
>!       this_rl->nregs = HARD_REGNO_NREGS (offsetted_regno, mode);
>!       this_rl->reginfo.regno = offsetted_regno;
>!       /* We're overriding - this must already have been allocated earlier.  */
>!       if (! this_rl->reginfo.allocated)
>! 	abort ();
>!     }
>  
>!   this_rl->override_in = gen_rtx_REG (mode, offsetted_regno);
This is definitely wrong for mixed-mode reloads.  override_in must
have rl->inmode, not rl->mode here.

>! static void
>! inherit_one_chain (struct inherit_chain *head)
>! {
>
>!   while (this != last)
>!     {
>!       struct insn_chain *chain;
>!       struct reload *rl = this->chain->rld + this->rli->nr;
>! 
>!       noninherited_use |= head->used_after;
Should be this instead of head.
>
>!       if (! noninherited_use && this->next_same == 0)
Should be last instead of 0.


>! choose_reload_regs (struct insn_chain *chain)
>  {
>!       rld[i].in = chain->rld[i].override_in;
>!       if (chain->rld[i].override_out)
>!       rld[i].in = chain->rld[i].override_out;
I assume this should be out instead of in.  However, override_out
is never set to anything nonzero, so I'm not sure what this is for ...

  
>*************** emit_input_reload_insns (struct insn_cha
>*** 6136,6580 ****
Here you changed the old test  
>    /* Auto-increment addresses must be reloaded in a special way.  */
>!   if (rl->out && ! rl->out_reg)
to
>    /* Auto-increment addresses must be reloaded in a special way.  */
>!   if (rl->inc)
This looks wrong (and breaks on s390); according to reload.h

  /* Positive amount to increment or decrement by if
     reload_in is a PRE_DEC, PRE_INC, POST_DEC, POST_INC.
     Ignored otherwise (don't assume it is zero).  */
  int inc;

And indeed there are paths in find_reload that set inc to nonzero values
even in the absence of preincrement (s390 doesn't even *have* that!).


Bye,
Ulrich

-- 
  Dr. Ulrich Weigand
  Linux on zSeries Development
  Ulrich.Weigand@de.ibm.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]