What to do with new-ra for GCC 4.0

[Bernd Schmidt]

[CC'ing gcc-patches where I should have sent the thing in the first 
place...]

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

Wow, thanks for the detailed review.

> 
>>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?

Possibly.  I just didn't want to have to deal with this problem for 
every new cross compiler I tried.

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

Well, that's a good start.

[secondary reload lifetimes]

> 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.

Hmm.  Examples?  There's an abort in push_secondary_reload for this 
case, but it seems like it has been slightly relaxed in the last 5 
years...  What could this behaviour possibly be useful for?  Are these 
just cases where you need a special insn but not an extra register?

 > 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 think we might need both, but possibly we have to make 
"rl->reginfo.birth--" conditional on "secondary_icode == -1".

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

>>!   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.

Fixed.

>>! 	  if (GET_MODE (insn) == SImode)
> 
> Should be copy, not insn.

Fixed.

[Mixed-mode reload issues]

I need more time to think about those.

>>!       noninherited_use |= head->used_after;
> 
> Should be this instead of head.
> 
>>!       if (! noninherited_use && this->next_same == 0)
> 
> Should be last instead of 0.

>>!       if (chain->rld[i].override_out)
>>!       rld[i].in = chain->rld[i].override_out;
> 
> I assume this should be out instead of in.

All three fixed.

 > However, override_out
> is never set to anything nonzero, so I'm not sure what this is for ...

This can probably go away.

>>*************** 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;

Yes, as I mentioned in my previous mail, autoinc reloads aren't handled 
too gracefully yet (i.e. things break).

The thing is, "rl->out && ! rl->out_reg" is a really bizarre way to test 
for autoincrement.  This has always irritated me, and I wanted to clean 
it up a little.  I didn't realize that it can be nonzero in non-autoinc 
cases.

> 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!).

Then maybe we should fix those paths.


Bernd