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

[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

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