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 31, 2013 4:14 AM
> To: Zhenqiang Chen
> Cc: Richard Earnshaw; 'Richard Biener'; GCC Patches
> Subject: Re: [PATCH 1/n] Add conditional compare support
> 
> > +/* RCODE0, RCODE1 and a valid return value should be enum rtx_code.
> > +   TCODE should be enum tree_code.
> > +   Check whether two compares are a valid combination in the target to
> generate
> > +   a conditional compare.  If valid, return the new compare after
> combination.
> > +   */
> > +DEFHOOK
> > +(legitimize_cmp_combination,
> > + "This target hook returns the dominance compare if the two compares
> > +are\n\ a valid combination.  This target hook is required only when
> > +the target\n\ supports conditional compare, such as ARM.",  int, (int
> > +rcode0, int rcode1, int tcode),
> > + default_legitimize_cmp_combination)
> > +
> > +/* RCODE0, RCODE1 and a valid return value should be enum rtx_code.
> > +   TCODE should be enum tree_code.
> > +   Check whether two compares are a valid combination in the target to
> generate
> > +   a conditional compare.  If valid, return the new compare after
> combination.
> > +   The difference from legitimize_cmp_combination is that its first
> compare is
> > +   the result of a previous conditional compare, which leads to more
> constrain
> > +   on it, since no way to swap the two compares.  */ DEFHOOK
> > +(legitimize_ccmp_combination,  "This target hook returns the
> > +dominance compare if the two compares are\n\ a valid combination.
> > +This target hook is required only when the target\n\ supports
> > +conditional compare, such as ARM.",  int, (int rcode0, int rcode1,
> > +int tcode),
> > + default_legitimize_ccmp_combination)
> > +
> 
> Why do these hooks still exist?  They should be redundant with ...

They are not necessary. Will remove them.

> > +/* CMP0 and CMP1 are two compares.  USED_AS_CC_P indicates whether
> the target
> > +   is used as CC or not.  TCODE should be enum tree_code.
> > +   The hook will return a condtional compare RTX if all checkes are
> > +OK.  */ DEFHOOK (gen_ccmp_with_cmp_cmp,  "This target hook returns a
> > +condtional compare RTX if the two compares are\n\ a valid
> > +combination.  This target hook is required only when the target\n\
> > +supports conditional compare, such as ARM.",  rtx, (gimple cmp0,
> > +gimple cmp1, int tcode, bool used_as_cc_p),
> > + default_gen_ccmp_with_cmp_cmp)
> > +
> > +/* CC is the result of a previous conditional compare.  CMP1 is a compare.
> > +   USED_AS_CC_P indicates whether the target is used as CC or not.
> > +   TCODE should be enum tree_code.
> > +   The hook will return a condtional compare rtx if all checkes are
> > +OK.  */ DEFHOOK (gen_ccmp_with_ccmp_cmp,  "This target hook returns
> a
> > +condtional compare RTX if the CC and CMP1 are\n\ a valid combination.
> > +This target hook is required only when the target\n\ supports
> > +conditional compare, such as ARM.",  rtx, (rtx cc, gimple cmp1, int
> > +tcode, bool used_as_cc_p),
> > + default_gen_ccmp_with_ccmp_cmp)
> > +
> 
> ... these.
> 
> Why in the world are you passing down gimple to the backends?  The
> expand_operands done therein should have been done in expr.c.
> The hooks are still not what I suggested, particularly
> gen_ccmp_with_cmp_cmp is what I was trying to avoid, passing down two
> initial compares like that.
> 
> To be 100% explicit this time, I think the hooks should be

Thank you very much!
 
> DEFHOOK
> (gen_ccmp_first,
>  "This function emits a comparison insn for the first of a sequence of\n\
> conditional comparisions.  It returns a comparison expression appropriate\n\
> for passing to @code{gen_ccmp_next} or to @code{cbranch_optab}.",  rtx,
> (int code, rtx op0, rtx op1),
>  NULL)
> DEFHOOK
> (gen_ccmp_next,
>  "This function emits a conditional comparison within a sequence of\n\
> conditional comparisons.  The @code{prev} expression is the result of a\n\
> prior call to @code{gen_ccmp_first} or @code{gen_ccmp_next}.  It may
> return\n\ @code{NULL} if the combination of @code{prev} and this
> comparison is\n\ not supported, otherwise the result must be appropriate
> for passing to @code{gen_ccmp_next} or @code{cbranch_optab}.",  rtx, (rtx
> prev, int code, rtx op0, rtx op1),
>  NULL)

gen_ccmp_first and gen_ccmp_next are better. But need some improvement.

With two compares, we might swap the order of the two compares to get a valid combination. This is what current ARM does (Please refer arm_select_dominace_cc_mode, cmp_and, cmp_ior, and_scc_scc and ior_scc_scc). To improve gen_ccmp_first, we need another function/hook to determine which compare is done first. I will double check ARM backend to avoid hook.

Hook gen_ccmp_next needs one more parameter to indicate AND/IOR since they will generate different instructions.  

I will update the patch.

> All of your existing tests for HAVE_ccmp should be replaced with
> 
>   if (targetm.gen_ccmp_first == NULL)
>     return; /* No ccmp supported. */
>   gcc_checking_assert(targetm.gen_ccmp_next != NULL);

Thanks. I will update it.

-Zhenqiang