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

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

Patch is updated according to your comments. Main changes are:
* Add two hooks: legitimize_cmp_combination and legitimize_ccmp_combination
* Improve document.

Thanks!
-Zhenqiang

> -----Original Message-----
> From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-
> owner@gcc.gnu.org] On Behalf Of Zhenqiang Chen
> Sent: Thursday, October 24, 2013 4:11 PM
> To: 'Richard Henderson'; Richard Earnshaw; 'Richard Biener'
> Cc: GCC Patches
> Subject: RE: [PATCH 1/n] Add conditional compare support
> 
> 
> > -----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
> 
> 
>

Attachment: ccmp-hook.patch
Description: Binary data