[PATCH 5/5] single_set takes an insn

[David Malcolm <dmalcolm at redhat dot com>]

On Tue, 2014-09-09 at 09:53 -0600, Jeff Law wrote:
> On 09/08/14 15:29, David Malcolm wrote:
> > gcc/ChangeLog:
[...]
> > 	* config/avr/avr.c (avr_out_plus): Add checked cast to rtx_insn *
> > 	when invoking single_set in region guarded by INSN_P.
> > 	(avr_out_bitop): Likewise.
> > 	(_reg_unused_after): Introduce local rtx_sequence * "seq" in
> > 	region guarded by GET_CODE check, using methods to strengthen
> > 	local "this_insn" from rtx to rtx_insn *, and for clarity.
> > 	* config/avr/avr.md (define_insn_and_split "xload8<mode>_A"):
> > 	Strengthen local "insn" from rtx to rtx_insn *.
> > 	(define_insn_and_split "xload<mode>_A"): Likewise.
[...]

> > diff --git a/gcc/config/avr/avr.c b/gcc/config/avr/avr.c
> > index 70d5db5..e749793 100644
> > --- a/gcc/config/avr/avr.c
> > +++ b/gcc/config/avr/avr.c
> > @@ -6769,7 +6769,7 @@ avr_out_plus (rtx insn, rtx *xop, int *plen, int *pcc, bool out_label)
> >     int cc_plus, cc_minus, cc_dummy;
> >     int len_plus, len_minus;
> >     rtx op[4];
> > -  rtx xpattern = INSN_P (insn) ? single_set (insn) : insn;
> > +  rtx xpattern = INSN_P (insn) ? single_set (as_a <rtx_insn *> (insn)) : insn;
> Whee, presumably a new item for the TODO list :-)  "insn" here could be 
> strengthened to an rtx_insn.

[dropping DJ from CC as this doesn't relate to rl78]
[adding Denis to CC due to avr-related discussion]

The reason for the as_a <rtx_insn *> here is that avr_out_plus and
avr_out_bitop are mostly called with insns [1], but are also called with
SET patterns [2], which is what the changed line above is meant to
handle.

Perhaps a fix could be to split each into two functions, perhaps:

static avr_out_plus_set
extern avr_out_plus_insn

with the latter calling the former (the former is only used from
avr_out_round).

I did start looking at this, but the logic in avr_out_plus makes it
non-trivial:

  6766  const char*
  6767  avr_out_plus (rtx insn, rtx *xop, int *plen, int *pcc, bool out_label)
  6768  {
  6769    int cc_plus, cc_minus, cc_dummy;
  6770    int len_plus, len_minus;
  6771    rtx op[4];
  6772    rtx xpattern = INSN_P (insn) ? single_set (as_a <rtx_insn *> (insn)) : insn;
  6773    rtx xdest = SET_DEST (xpattern);
  6774    enum machine_mode mode = GET_MODE (xdest);
  6775    enum machine_mode imode = int_mode_for_mode (mode);
  6776    int n_bytes = GET_MODE_SIZE (mode);
  6777    enum rtx_code code_sat = GET_CODE (SET_SRC (xpattern));
  6778    enum rtx_code code
  6779      = (PLUS == code_sat || SS_PLUS == code_sat || US_PLUS == code_sat
  6780         ? PLUS : MINUS);
  6781  
  6782    if (!pcc)
  6783      pcc = &cc_dummy;
  6784  
  6785    /* PLUS and MINUS don't saturate:  Use modular wrap-around.  */
  6786  
  6787    if (PLUS == code_sat || MINUS == code_sat)
  6788      code_sat = UNKNOWN;
  6789  
  6790    if (n_bytes <= 4 && REG_P (xop[2]))
  6791      {
  6792        avr_out_plus_1 (xop, plen, code, pcc, code_sat, 0, out_label);
  6793        return "";
  6794      }
  6795  
  6796    if (8 == n_bytes)
  6797      {
  6798        op[0] = gen_rtx_REG (DImode, ACC_A);
  6799        op[1] = gen_rtx_REG (DImode, ACC_A);
  6800        op[2] = avr_to_int_mode (xop[0]);
  6801      }
  6802    else
  6803      {
  6804        if (!REG_P (xop[2])
  6805            && !CONST_INT_P (xop[2])
  6806            && !CONST_FIXED_P (xop[2]))
  6807          {
  6808            return avr_out_plus_symbol (xop, code, plen, pcc);
  6809          }
  6810  
  6811        op[0] = avr_to_int_mode (xop[0]);
  6812        op[1] = avr_to_int_mode (xop[1]);
  6813        op[2] = avr_to_int_mode (xop[2]);
  6814      }
  6815  
  6816    /* Saturations and 64-bit operations don't have a clobber operand.
  6817       For the other cases, the caller will provide a proper XOP[3].  */
  6818  
  6819    xpattern = INSN_P (insn) ? PATTERN (insn) : insn;
  6820    op[3] = PARALLEL == GET_CODE (xpattern) ? xop[3] : NULL_RTX;

Note how in line 6773 we use SET_DEST to get "xdest", and then proceed
to use xdest, so presumably insn was a SET, or an insn containing a SET
(perhaps within a PARALLEL) at line 6772.

But in line 6819 we go back to "insn" and either treat it as a pattern,
or re-extract the pattern, and then at line 6820 consider that it could
be a PARALLEL.

Given that lines 6819 onwards do a bunch of additional stuff, it seemed
easiest for now to simply handle both the pattern and insn cases in the
same function, rather than try to split them.  Of course, this would
need to be split out if we go ahead with the "insns are no longer a
subclass of rtx" idea.

> I won't call them all out since we can grep for "as_a <rtx_insn" to find 
> these in the future.


> OK for the trunk,
Thanks; r215089, fwiw.

Dave

[1] from avr_notice_update_cc, from avr_adjust_insn_length and from
clauses in the .md file
[2] once in each case, from avr_out_round, for both _plus and _bitop.