Re: [PATCH][LRA]Don't generate reload for output scratch operand from reload instruction.

[Renlin Li <renlin dot li at foss dot arm dot com>]

Hi Vladimir,

Thank you for explain it.
I have a few comments inlined.

On 26/02/16 23:54, Vladimir Makarov wrote:
> Thanks for working on this and providing a good description of the 
> problem.  Could you fill a PR and provide a test even if you can not 
> reduce it.

I will fill a PR. Try to reduce a test case. As it's triggered by my 
local change to gcc, I cannot guarantee it.
Anyway, I am quite happy to test your fix when you have one.

> As for the scratch.  As I understand the scratch was introduced for 
> operands which will not require any resources (memory or a new 
> register) for some insn alternatives.  If we use pseudo for this, it 
> will always need memory or a register.  The typical constraint for 
> scratch is "r,X" or "0r".  So I guess using just "&r" for scratch is a 
> bad practice.  Still for compatibility I think we should implement the 
> same reload behaviour for this case too.

Actually (clobber (match_scratch:MODE x "=r")) also triggers this ICE. 
The early clobber modifier here doesn't really matter.
the purpose of this pattern is to reserve a pseudo register for use as a 
temporary.
The "=" modifier is required for MATCH_SCRATCH expression. Otherwise, it 
will error "missing output reload"
That why (set scratch, RXX) is generated.

> I believe we should use the same technique -- changing scratches to 
> pseudo and back at the end of LRA if they don't need a register.  It 
> will solve also a possible problem for correct scratch generation 
> during LRA.
>
> I am going to work on this problem on the next week.  A test case 
> would be a help for me.
> > gcc/ChangeLog:
> >
> > 2016-02-26  Renlin Li<renlin.li@arm.com>
> >
> >     * lra-constraints.c (curr_insn_transform): Don't generate reload for
> >     output scratch operand.
> Sorry, I can not accept the patch as I'd like to provide a better 
> solution I described above.  The patch is also wrong for unused 
> non-scratch operands.  They still should be reloaded if they do not 
> satisfy their constraints even if they are not used later.

I think still it will be reload according to the code logic here:

if (get_reload_reg (type, mode, old, goal_alt[i],
                  loc != curr_id->operand_loc[i], "", &new_reg)
          && type != OP_OUT)
        {
          push_to_sequence (before);
          lra_emit_move (new_reg, old);
          before = get_insns ();
          end_sequence ();
        }
      *loc = new_reg; ------------->>>>>>>>> the original operand will 
be replaced by a reload reg.
      if (type != OP_IN
          /* Don't generate reload for output scratch operand.  */
          && GET_CODE (old) != SCRATCH
          && find_reg_note (curr_insn, REG_UNUSED, old) == NULL_RTX)


a reload register will be generated to replace the old operand in the 
original rtx pattern to satisfy their constraints.
Later, it will check, if this operand is an ouput operand which will be 
used later, another insn will be generated to
move newly generate pseudo into old operand.

The patch is to add one more condition to this final insn generation.

Regards,
Renlin