Re: [RFC][PATCH] Preferred rename register in regrename pass

[Bernd Schmidt <bschmidt@redhat.com>]

On 09/17/2015 04:38 PM, Robert Suchanek wrote:
> We came across a situation for MIPS64 where moves for sign-extension were
> not converted into a nop because of IRA spilled some of the allocnos and
> assigned different hard register for the output operand in the move.
> LRA is not fixing this up as most likely the move was not introduced by
> the LRA itself.  I found it hard to fix this in LRA and looked at
> an alternative solution where regrename pass appeared to be the best candidate.

For reference, please post examples of the insn pattern(s) where you 
would hope to get an improvement. Do they use matching constraints 
between the input and output operands in at least one alternative?

So this does look like something that could be addressed in regrename, 
but I think the patch is not quite the way to do it.

> +/* Return a preferred rename register for HEAD.  */

Function comments ideally ought to be a little more detailed. Preferred 
how and why?

> +static int
> +find_preferred_rename_reg (du_head_p head)
> +{
> +  struct du_chain *this_du;
> +  int preferred_reg = -1;
> +
> +  for (this_du = head->first; this_du; this_du = this_du->next_use)

This loop seems to search for the insn where the chain terminates (i.e. 
the register dies). It seems strange to do this here rather than during 
the initial scan in record_out_operands where we visit every insn and 
already look for REG_DEAD notes.

> +      rtx note;
> +      insn_rr_info *p;
> +
> +      /* The preferred rename register is an output register iff an input
> +	 register dies in an instruction but the candidate must be validated by
> +	 check_new_reg_p.  */
> +      for (note = REG_NOTES (this_du->insn); note; note = XEXP (note, 1))
> +	if (insn_rr.exists()
> +	    && REG_NOTE_KIND (note) == REG_DEAD
> +	    && REGNO (XEXP (note, 0)) == head->regno
> +	    && (p = &insn_rr[INSN_UID (this_du->insn)])
> +	    && p->op_info)
> +	  {
> +	    int i;
> +	    for (i = 0; i < p->op_info->n_chains; i++)
> +	      {
> +		struct du_head *next_head = p->op_info->heads[i];
> +		if (head != next_head)

Here you're not actually verifying the chosen preferred reg is an 
output? Is the use of plain "p->op_info" (which is actually an array) 
intentional as a guess that operand 0 is the output? I'm not thrilled 
with this, and at the very least it should be "p->op_info[0]." to avoid 
reader confusion.
It's also not verifying that this is indeed a case where choosing a 
preferred reg has a beneficial effect at all.

The use of insn_rr would probably also be unnecessary if this was done 
during the scan phase.

> +		    preferred_reg = next_head->regno;

The problem here is that there's an ordering issue. What if next_head 
gets renamed afterwards? The choice of preferred register hasn't bought 
us anything in that case.

For all these reasons I'd suggest a different approach, looking for such 
situations during the scan. Try to detect a situation where
  * we have a REG_DEAD note for an existing chain
  * the insn fulfils certain conditions (i.e. it's a move, or maybe one
    of the alternatives has a matching constraint). After all, there's
    not much point in tying if the reg that dies was used in a memory
    address.
  * a new chain is started for a single output
Then, instead of picking a best register, mark the two chains as tied. 
Then, when choosing a rename register, see if a tied chain already was 
renamed, and try to pick the same register first.

> @@ -1826,7 +1900,7 @@ regrename_optimize (void)
>     df_analyze ();
>     df_set_flags (DF_DEFER_INSN_RESCAN);
>
> -  regrename_init (false);
> +  regrename_init (true);

It would be good to avoid this as it makes the renamer more expensive. I 
expect that if you follow the strategy described above, this won't be 
necessary.

> -  struct du_chain *chains[MAX_REGS_PER_ADDRESS];
> -  struct du_head *heads[MAX_REGS_PER_ADDRESS];
> +  vec<struct du_chain *> chains;
> +  vec<struct du_head *> heads;

Given that MAX_REGS_PER_ADDRESS tends to be 1 or 2 this appears to make 
things more heavyweight, especially with the extra loop needed to free 
the vecs. If possible, try to avoid this. (Again, AFAICS this 
information shouldn't really be necessary for what you're trying to do).


Bernd