RE: [PATCH 1/n] Add conditional compare support

["Zhenqiang Chen" <zhenqiang dot chen at arm dot com>]

> -----Original Message-----
> From: Richard Henderson [mailto:rth@redhat.com]
> Sent: Thursday, October 24, 2013 12:14 AM
> To: Zhenqiang Chen; Richard Earnshaw; 'Richard Biener'
> Cc: GCC Patches
> Subject: Re: [PATCH 1/n] Add conditional compare support
> 
> > +static enum rtx_code
> > +arm_ccmode_to_code (enum machine_mode mode) {
> > +  switch (mode)
> > +    {
> > +    case CC_DNEmode:
> > +      return NE;
> 
> Why would you need to encode comparisons in CCmodes?
> That looks like a mis-design to me.

The CCmodes are used to check whether the result of a previous conditional compare can combine with current compare. By changing it to rtx_code, I can reuse current code (arm_select_dominance_cc_mode_1) to check it.
The CCmodes are also used to emit the "condition code" for a conditional execution. E.g.

  CC1 (CC_DGEmode) = CCMP (a >= 0, b >= 0)
==> cmp a, #0
         cmpge b, #0
  CC2 = CCMP ( CC1 != 0, c >= 0)
==> cmpge c, #0
The "ge" is from the mode of CC1 in "CC1 != 0". And the mode of CC2 is not necessary the same as CC1.
 
> > +Conditional compare instruction.  Operand 2 and 5 are RTLs which
> > +perform two comparisons.  Operand 1 is AND or IOR, which operates on
> > +the result of Operand 2 and 5.  Operand 0 is the result of operand 1.
> > +
> > +A typical @code{ccmp} pattern looks like
> > +
> > +@smallexample
> > +(define_expand "ccmp"
> > +  [(set (match_operand 0 "register_operand" "")
> > +        (match_operator 1 ""
> > +         [(match_operator:SI 2 "comparison_operator"
> > +          [(match_operand:SI 3 "register_operand")
> > +           (match_operand:SI 4 "register_operand")])
> > +         (match_operator:SI 5 "comparison_operator"
> > +          [(match_operand:SI 6 "register_operand")
> > +           (match_operand:SI 7 "register_operand")])]))]
> > +  ""
> > +  "@dots{}")
> > +@end smallexample
> 
> This documentation is inadequate.  What sorts of input combinations are
> valid?

It depends on target. I will update it with your later comments on
* How to use cmp_hook and ccmp_hook to check valid combinations.
* How to write patterns to support more than two compares.

>  How is the middle-end expected to compose more than two compares, as
> you describe in the mail?  What mode should the middle-end use to allocate
> that output register?

The ccmp_candidate_p and expand_ccmp_expr_* are using recursively algorithm. One conditional compare can have two and only two compares. But if the operand is the result of a previous conditional compare, we have more than two compares. i.e.

CC1 = CCMP (CMP1, CMP2)
CC2 = CCMP (CC1 != 0, CMP3)
...
CCn = CCMP (CCn-1 != 0, CMPn+1)

And these are the representations we want to have on TREE/GIMPLE after front-end in my original patch.
http://gcc.1065356.n5.nabble.com/PATCH-1-n-Add-conditional-compare-support-td961953.html

> The only thing that makes a sort of sense to me is something akin to how the
> old cmp + bcc patterns worked.  Except that's not especially clean, and
> there's a reason we got rid of them.
> 
> I think the only clean interface is going to be a new hook or hooks.  The
> return value of the hook with be an rtx akin to the PTEST value returned by
> prepare_cmp_insn.  Which is a proper input to cbranch/cstore/etc.
> 
> I might think that two hooks would be appropriate because it might make the
> ccmp hook easier.  I.e.
> 
>     res = targetm.cmp_hook(...);
>     while (more)
>       res = targetm.ccmp_hook(res, ...);
> 
> For combinations that can't be implemented with ccmp, the hook can return
> null.

Yip.  This will be more efficient. I will update the patch.
In current patch, arm_select_dominance_cc_mode is the "cmp_hook" and arm_select_dominance_ccmp_mode is the "ccmp_hook".
 
> > +;; The first compare in this pattern is the result of a previous CCMP.
> > +;; We can not swap it.  And we only need its flag.
> > +(define_insn "*ccmp_and"
> > +  [(set (match_operand 6 "dominant_cc_register" "")
> > +	(compare
> > +	 (and:SI
> > +	  (match_operator 4 "expandable_comparison_operator"
> > +	   [(match_operand 0 "dominant_cc_register" "")
> > +	    (match_operand:SI 1 "arm_add_operand" "")])
> > +	  (match_operator:SI 5 "arm_comparison_operator"
> > +	   [(match_operand:SI 2 "s_register_operand"
> > +	        "l,r,r,r,r")
> > +	    (match_operand:SI 3 "arm_add_operand"
> > +	        "lPy,rI,L,rI,L")]))
> > +	 (const_int 0)))]
> > +  "TARGET_32BIT"
> > +  "*
> > +  {
> > +    static const char *const cmp2[2] =
> > +    {
> > +      \"cmp%d4\\t%2, %3\",
> > +      \"cmn%d4\\t%2, #%n3\"
> > +    };
> > +    static const char *const ite = \"it\\t%d4\";
> > +    static const int cmp_idx[9] = {0, 0, 1, 0, 1};
> > +
> > +    if (TARGET_THUMB2) {
> > +      output_asm_insn (ite, operands);
> > +    }
> > +    output_asm_insn (cmp2[cmp_idx[which_alternative]], operands);
> > +    return \"\";
> > +  }"
> 
> I would think simply splitting to a normal cond_exec insn post reload would
> be significantly cleaner than this.

The codes are copied from "*cmp_and ". I will try to simplify to normal cond_exec insn later.

> And for future reference, don't use '"*', just use '{'.  That way you don't have
> to escape the embedded quotes, etc.

Thanks. I will update it.

-Zhenqiang