Re: [PATCH][sched-deps] Generalise usage of macro fusion to work on any two insns

[Maxim Kuvyrkov <maxim dot kuvyrkov at linaro dot org>]

On Jul 10, 2014, at 8:00 PM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:

> 
> On 30/06/14 21:39, Jeff Law wrote:
>> On 06/27/14 02:29, Kyrill Tkachov wrote:
>>> Hi all,
>>> 
>>> This patch generalises the TARGET_MACRO_FUSION_PAIR_P hook usage to work
>>> on more than just
>>> compares and conditional branches for which it was initially designed
>>> for (for x86).
>>> 
>>> There are some instructions in arm and aarch64 that can be fused
>>> together when they're back to back in the instruction stream and I'd
>>> like to use this hook to keep them together.
>>> 
>>> I'll post an implementation of TARGET_MACRO_FUSION_PAIR_P for arm and
>>> aarch64 shortly...
>>> 
>>> Bootstrapped and tested on x86, aarch64-none-linux-gnu and
>>> arm-none-linux-gnueabihf.
>>> 
>>> Ok for trunk?

The patch looks good to me, but some cleanup suggestions below.

> commit e36b8977738dbe3f63445199710ca627ab37e243
> Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
> Date:   Fri Jun 13 11:41:41 2014 +0100
> 
>     [sched-deps] Generalise macro fusion hook usage
> 
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index 8046c67..7dd2ce5 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -25817,6 +25817,9 @@ ix86_macro_fusion_pair_p (rtx condgen, rtx condjmp)
>    rtx compare_set = NULL_RTX, test_if, cond;
>    rtx alu_set = NULL_RTX, addr = NULL_RTX;
>  
> +  if (!any_condjump_p (condjmp))
> +    return false;
> +
>    if (get_attr_type (condgen) != TYPE_TEST
>        && get_attr_type (condgen) != TYPE_ICMP
>        && get_attr_type (condgen) != TYPE_INCDEC
> diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
> index 7cafc8b..c01a8a6 100644
> --- a/gcc/sched-deps.c
> +++ b/gcc/sched-deps.c
> @@ -2820,35 +2820,48 @@ sched_analyze_2 (struct deps_desc *deps, rtx x, rtx insn)
>      sched_deps_info->finish_rhs ();
>  }
>  
> -/* Try to group comparison and the following conditional jump INSN if
> -   they're already adjacent. This is to prevent scheduler from scheduling
> -   them apart.  */
> +/* Try to group two fuseable insns together to prevent scheduler
> +   from scheduling them apart.  */
>  
>  static void
>  try_group_insn (rtx insn)

Please rename try_group_insn to sched_macro_fuse_insns.  The call is predicated to try_group_insn is predicated on targetm.sched.macro_fusion_p, so this code will not be used for any other kinds of fusion -- might as well just state that in the name,.

>  {
> -  unsigned int condreg1, condreg2;
> -  rtx cc_reg_1;
>    rtx prev;
>  
> -  if (!any_condjump_p (insn))
> +  if (!targetm.sched.macro_fusion_p ())
>      return;

This is a no-op since there is a check on the upper level.  Please remove.

>  
> -  targetm.fixed_condition_code_regs (&condreg1, &condreg2);
> -  cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
> -  prev = prev_nonnote_nondebug_insn (insn);
> -  if (!reg_referenced_p (cc_reg_1, PATTERN (insn))
> -      || !prev
> -      || !modified_in_p (cc_reg_1, prev))
> -    return;
> +  if (any_condjump_p (insn))
> +    {
> +      unsigned int condreg1, condreg2;
> +      rtx cc_reg_1;
> +      targetm.fixed_condition_code_regs (&condreg1, &condreg2);
> +      cc_reg_1 = gen_rtx_REG (CCmode, condreg1);
> +      prev = prev_nonnote_nondebug_insn (insn);
> +      if (!reg_referenced_p (cc_reg_1, PATTERN (insn))
> +	  || !prev
> +	  || !modified_in_p (cc_reg_1, prev))
> +	return;
>  
> -  /* Different microarchitectures support macro fusions for different
> -     combinations of insn pairs.  */
> -  if (!targetm.sched.macro_fusion_pair_p
> -      || !targetm.sched.macro_fusion_pair_p (prev, insn))
> -    return;
> +      if (targetm.sched.macro_fusion_pair_p (prev, insn))
> +        SCHED_GROUP_P (insn) = 1;
> +    }
> +  else
> +    {
> +      rtx insn_set = single_set (insn);
> +
> +      prev = prev_nonnote_nondebug_insn (insn);
> +      if (prev
> +          && insn_set
> +          && single_set (prev)
> +          && modified_in_p (SET_DEST (insn_set), prev)

Invert the check (as done in the upper if-clause) and cut it here.  Then you can use a single unified

if (targetm.sched.macro_fusion_pair_p (prev, insn))
  SCHED_GROUP_P (insn) = 1;

as the final statement of the function.

Thank you,

--
Maxim Kuvyrkov
www.linaro.org