RE: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)

[Zhenqiang Chen]

> -----Original Message-----
> From: Richard Henderson [mailto:r...@redhat.com]
> Sent: Tuesday, November 25, 2014 5:25 PM
> To: Zhenqiang Chen
> Cc: Marcus Shawcroft; gcc-patches@gcc.gnu.org
> Subject: Re: [PATCH, AARCH64] Fix ICE in CCMP (PR64015)
> 
> On 11/25/2014 09:41 AM, Zhenqiang Chen wrote:
> > I want to confirm with you two things before I rework it.
> > (1) expand_insn needs an optab_handler as input. Do I need to define a
> ccmp_optab with different mode support in optabs.def?
> 
> No, look again: expand_insn needs an enum insn_code as input.  Since this is
> the backend, you can use any icode name you like, which means that you can
> use CODE_FOR_ccmp_and etc directly.
> 
> > (2) To make sure later operands not clobber CC, all operands are expanded
> before ccmp-first in current implementation. If taking tree/gimple as input,
> what's your preferred logic to guarantee CC not clobbered?
> 
> Hmm.  Perhaps the target hook will need to output two sequences, each of
> which will be concatenated while looping around the calls to gen_ccmp_next.
> The first sequence will be operand preparation and the second sequence will
> be ccmp generation.
> 
> Something like
> 
> bool
> aarch64_gen_ccmp_start(rtx *prep_seq, rtx *gen_seq,
>                        int cmp_code, int bit_code,
>                        tree op0, tree op1) {
>   bool success;
> 
>   start_sequence ();
>   // Widen and expand operands
>   *prep_seq = get_insns ();
>   end_sequence ();
> 
>   start_sequence ();
>   // Generate the first compare
>   *gen_seq = get_insns ();
>   end_sequence ();
> 
>   return success;
> }
> 
> bool
> aarch64_gen_ccmp_next(rtx *prep_seq, rtx *gen_seq,
>                       rtx prev, int cmp_code, int bit_code,
>                       tree op0, tree op1) {
>   bool success;
> 
>   push_to_sequence (*prep_seq);
>   // Widen and expand operands
>   *prep_seq = get_insns ();
>   end_sequence ();
> 
>   push_to_sequence (*gen_seq);
>   // Generate the next ccmp
>   *gen_seq = get_insns ();
>   end_sequence ();
> 
>   return success;
> }
> 
> If there are ever any failures, the middle-end can simply discard the
> sequences.  If everything succeeds, it simply calls emit_insn on both
> sequences.

When there are more than one ccmps, it will be complexity to maintain the 
un-emitted sequences in a recursive algorithm. E.g.
     CC0 = CMP (a, b);
     CC1 = CCMP (NE (CC0, 0), CMP (e, f));
     ...
     CCn = CCMP (NE (CCn-1, 0), CMP (...));

I read the codes to expand cstoresi and cbranchsi. It just uses normal 
expand_operands. So I think we can keep expand_operands in middle-end.

For the start one, we do not need worry about it since its last step should 
compare to set CC. And it is easy to delete the insns when fail.
static rtx
aarch64_gen_ccmp_first (int code, rtx op0, rtx op1)
{
   ...  // init the vars

  op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode, unsignedp);
  op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode, unsignedp);
  if (!op0 || !op1)
    return NULL_RTX;

  cmp = gen_rtx_fmt_ee ((enum rtx_code) code, cmp_mode, op0, op1);
  target = gen_rtx_REG (CCmode, CC_REGNUM);

  create_output_operand (&ops[0], target, CCmode);
  create_fixed_operand (&ops[1], cmp);
  create_fixed_operand (&ops[2], op0);
  create_fixed_operand (&ops[3], op1);

  if (!maybe_expand_insn (icode, 4, ops))
    return NULL_RTX;

  return gen_rtx_REG (cc_mode, CC_REGNUM);
}

aarch64_gen_ccmp_next (rtx prev, int cmp_code, rtx op0, rtx op1, int bit_code)
{
   ...  // init the vars

  op0 = prepare_operand (icode, op0, 2, op_mode, cmp_mode, unsignedp);
  op1 = prepare_operand (icode, op1, 3, op_mode, cmp_mode, unsignedp);
  if (!op0 || !op1)
    return NULL_RTX;

  /* Check to make sure CC is not clobbered since prepare_operand might
     generates copy or mode convertion insns, although no test shows
     such insns clobber CC.  */
  ...

  cmp1 = gen_rtx_fmt_ee ((enum rtx_code) cmp_code, cmp_mode, op0, op1);
  cmp0 = gen_rtx_fmt_ee (NE, cmp_mode, prev, const0_rtx);

  target = gen_rtx_REG (cc_mode, CC_REGNUM);

  create_fixed_operand (&ops[0], prev);
  create_fixed_operand (&ops[1], target);
  create_fixed_operand (&ops[2], op0);
  create_fixed_operand (&ops[3], op1);
  create_fixed_operand (&ops[4], cmp0);
  create_fixed_operand (&ops[5], cmp1);

  if (!maybe_expand_insn (icode, 6, ops))
    return NULL_RTX;

  return target;
}

Does such change align with your comments?

Thanks!
-Zhenqiang