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

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 825 bytes --]

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?

2014-06-27  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
                     Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
     to any two insns.  Update comment.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: macro-fusion-midend.patch --]
[-- Type: text/x-patch; name=macro-fusion-midend.patch, Size: 2409 bytes --]

commit 1b096c837bf49dcfd9ff3e55168593d4a4aea48d
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/sched-deps.c b/gcc/sched-deps.c
index 7cafc8b..71e274d 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2820,35 +2820,57 @@ 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)
 {
-  unsigned int condreg1, condreg2;
-  rtx cc_reg_1;
   rtx prev;
 
-  if (!any_condjump_p (insn))
+  if (!targetm.sched.macro_fusion_pair_p)
     return;
 
-  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;
+
+      goto succ;
 
+    }
+  else
+    {
+      rtx set_dest = single_set (insn);
+
+      prev = prev_nonnote_nondebug_insn (insn);
+      if (prev
+	  && set_dest
+	  && single_set (prev))
+	{
+	  set_dest = SET_DEST (set_dest);
+
+	  if (!modified_in_p (set_dest, prev))
+	    return;
+
+	  goto succ;
+	}
+    }
+
+  return;
+
+ succ:
   /* 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;
-
-  SCHED_GROUP_P (insn) = 1;
+  if (prev && targetm.sched.macro_fusion_pair_p (prev, insn))
+    SCHED_GROUP_P (insn) = 1;
 }
 
 /* Analyze an INSN with pattern X to find all dependencies.  */

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

[Kyrill Tkachov]

On 27/06/14 09: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?
>
> 2014-06-27  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>                       Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
>       to any two insns.  Update comment.

Hmm, found a bug in this patch. Will respin...

Kyrill

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

[Jeff Law]

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?
>
> 2014-06-27  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>                      Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>      * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
>      to any two insns.  Update comment.
Isn't this going to end up calling the x86 specific macro_fusion_pair_p 
with a lot more insns than that function was previously prepared to handle?

In particular I'm concerned that the 2nd argument is going to be a 
non-jumping insn a lot more often.  Of particular concern is this code:


   test_if = SET_SRC (pc_set (condjmp));
   cond = XEXP (test_if, 0);
   ccode = GET_CODE (cond);

if CONDJMP is not a JUMP_INSN, pc_set is going to return NULL and XEXP 
(test_if, 0) will then fault.

I realize you bootstrapped on x86, but I suspect that whatever tuning 
you need to enable to really exercise this code wasn't on.

I think you can deal with this by putting

if (!any_condjump_p (condjmp)) at the start of the x86 specific
macro_fusion_pair_p is sufficient to address this issue.  It also 
ensures that we don't do a lot of unnecessary work in that function.

 From a general code structure standpoint, can you avoid this kind of 
structure:

if (any_condjmp_p (insn))
   {
     ...
     goto succ;
   }
else
   {
     ...
     goto succ
   }
return

succ:

Can you structure so that you return for all the cases where you don't 
want to set SCHED_GROUP_P from each arm?  Or go ahead and duplicate the 
SCHED_GROUP_P setting in each arm of the conditional.

jeff

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 3019 bytes --]


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?
>>
>> 2014-06-27  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>>                       Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>       * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
>>       to any two insns.  Update comment.
> Isn't this going to end up calling the x86 specific macro_fusion_pair_p
> with a lot more insns than that function was previously prepared to handle?
>
> In particular I'm concerned that the 2nd argument is going to be a
> non-jumping insn a lot more often.  Of particular concern is this code:
>
>
>     test_if = SET_SRC (pc_set (condjmp));
>     cond = XEXP (test_if, 0);
>     ccode = GET_CODE (cond);
>
> if CONDJMP is not a JUMP_INSN, pc_set is going to return NULL and XEXP
> (test_if, 0) will then fault.
>
> I realize you bootstrapped on x86, but I suspect that whatever tuning
> you need to enable to really exercise this code wasn't on.
>
> I think you can deal with this by putting
>
> if (!any_condjump_p (condjmp)) at the start of the x86 specific
> macro_fusion_pair_p is sufficient to address this issue.  It also
> ensures that we don't do a lot of unnecessary work in that function.
>
>   From a general code structure standpoint, can you avoid this kind of
> structure:
>
> if (any_condjmp_p (insn))
>     {
>       ...
>       goto succ;
>     }
> else
>     {
>       ...
>       goto succ
>     }
> return
>
> succ:
>
> Can you structure so that you return for all the cases where you don't
> want to set SCHED_GROUP_P from each arm?  Or go ahead and duplicate the
> SCHED_GROUP_P setting in each arm of the conditional.

Hi Jeff,

Thanks for the pointers, I've reworked the patch and it does look 
cleaner. I've made sure to run the x86 bootstrap with Haswell tuning and 
instrumented the code to make sure that the x86 macro fusion code was 
being exercised and it passed that fine.

How's that?

2014-07-10  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
             Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
     to any two insns.  Update comment.
     * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd arguments
     that are not conditional jumps.


>
> jeff
>
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: macro-fusion-midend.patch --]
[-- Type: text/x-patch; name=macro-fusion-midend.patch, Size: 2958 bytes --]

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)
 {
-  unsigned int condreg1, condreg2;
-  rtx cc_reg_1;
   rtx prev;
 
-  if (!any_condjump_p (insn))
+  if (!targetm.sched.macro_fusion_p ())
     return;
 
-  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)
+          && targetm.sched.macro_fusion_pair_p (prev, insn))
+        {
+            SCHED_GROUP_P (insn) = 1;
+        }
+
+    }
 
-  SCHED_GROUP_P (insn) = 1;
 }
 
 /* Analyze an INSN with pattern X to find all dependencies.  */

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

[Maxim Kuvyrkov]

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

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 5095 bytes --]


On 10/07/14 22:53, Maxim Kuvyrkov wrote:
> 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.

Thanks, here's an updated patch.
How's this?

2014-07-11  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
             Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
     to any two insns.  Update comment.  Rename to sched_macro_fuse_insns.
     (sched_analyze_insn): Update use of try_group_insn to
     sched_macro_fuse_insns.
     * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd arguments
     that are not conditional jumps.


>
>> 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
>
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: macro-fusion-midend.patch --]
[-- Type: text/x-patch; name=macro-fusion-midend.patch, Size: 3352 bytes --]

commit cb0584229d9247df805df35dc4c5bffbb839d59f
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 1b5cbeb..6951ddd 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -25820,6 +25820,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..2d9c834 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2820,35 +2820,45 @@ 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)
+sched_macro_fuse_insns (rtx insn)
 {
-  unsigned int condreg1, condreg2;
-  rtx cc_reg_1;
   rtx prev;
 
-  if (!any_condjump_p (insn))
-    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;
+
+      if (targetm.sched.macro_fusion_pair_p (prev, insn))
+        SCHED_GROUP_P (insn) = 1;
+    }
+  else
+    {
+      rtx insn_set = single_set (insn);
 
-  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;
+      prev = prev_nonnote_nondebug_insn (insn);
+      if (!prev
+          || !insn_set
+          || !single_set (prev)
+          || !modified_in_p (SET_DEST (insn_set), 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;
 
-  SCHED_GROUP_P (insn) = 1;
 }
 
 /* Analyze an INSN with pattern X to find all dependencies.  */
@@ -2877,7 +2887,7 @@ sched_analyze_insn (struct deps_desc *deps, rtx x, rtx insn)
   /* Group compare and branch insns for macro-fusion.  */
   if (targetm.sched.macro_fusion_p
       && targetm.sched.macro_fusion_p ())
-    try_group_insn (insn);
+    sched_macro_fuse_insns (insn);
 
   if (may_trap_p (x))
     /* Avoid moving trapping instructions across function calls that might

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

[Alexander Monakov]

On Fri, 11 Jul 2014, Kyrill Tkachov wrote:
> 
> On 10/07/14 22:53, Maxim Kuvyrkov wrote:
> > The patch looks good to me, but some cleanup suggestions below.
> 
> Thanks, here's an updated patch.
> How's this?

You need to remove 'if (targetm. ...) SCHED_GROUP_P (insn) = 1;' from the
first if branch, keeping only one SCHED_GROUP_P assignment at the end of the
function.

Alexander

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

[Kyrill Tkachov]

On 11/07/14 14:20, Alexander Monakov wrote:
> On Fri, 11 Jul 2014, Kyrill Tkachov wrote:
>> On 10/07/14 22:53, Maxim Kuvyrkov wrote:
>>> The patch looks good to me, but some cleanup suggestions below.
>> Thanks, here's an updated patch.
>> How's this?
> You need to remove 'if (targetm. ...) SCHED_GROUP_P (insn) = 1;' from the
> first if branch, keeping only one SCHED_GROUP_P assignment at the end of the
> function.

Yes, your're right, I had missed that. Will do.

Thanks,
Kyrill

>
> Alexander
>

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

[Kyrill Tkachov]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]


On 11/07/14 14:20, Alexander Monakov wrote:
> On Fri, 11 Jul 2014, Kyrill Tkachov wrote:
>> On 10/07/14 22:53, Maxim Kuvyrkov wrote:
>>> The patch looks good to me, but some cleanup suggestions below.
>> Thanks, here's an updated patch.
>> How's this?
> You need to remove 'if (targetm. ...) SCHED_GROUP_P (insn) = 1;' from the
> first if branch, keeping only one SCHED_GROUP_P assignment at the end of the
> function.
>
> Alexander

Thanks for the pointer, I had hurried a bit.
Here is the updated patch.

Kyrill

2014-07-14  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
             Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
     to any two insns.  Update comment.  Rename to sched_macro_fuse_insns.
     (sched_analyze_insn): Update use of try_group_insn to
     sched_macro_fuse_insns.
     * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd arguments
     that are not conditional jumps.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: macro-fusion-midend.patch --]
[-- Type: text/x-patch; name=macro-fusion-midend.patch, Size: 3253 bytes --]

commit 643a8658f1788de2301d9d6a0457979c06afbdf9
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 1b5cbeb..6951ddd 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -25820,6 +25820,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..ae2fff2 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2820,35 +2820,42 @@ 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)
+sched_macro_fuse_insns (rtx insn)
 {
-  unsigned int condreg1, condreg2;
-  rtx cc_reg_1;
   rtx prev;
 
-  if (!any_condjump_p (insn))
-    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;
+    }
+  else
+    {
+      rtx insn_set = single_set (insn);
 
-  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;
+      prev = prev_nonnote_nondebug_insn (insn);
+      if (!prev
+          || !insn_set
+          || !single_set (prev)
+          || !modified_in_p (SET_DEST (insn_set), 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;
 
-  SCHED_GROUP_P (insn) = 1;
 }
 
 /* Analyze an INSN with pattern X to find all dependencies.  */
@@ -2877,7 +2884,7 @@ sched_analyze_insn (struct deps_desc *deps, rtx x, rtx insn)
   /* Group compare and branch insns for macro-fusion.  */
   if (targetm.sched.macro_fusion_p
       && targetm.sched.macro_fusion_p ())
-    try_group_insn (insn);
+    sched_macro_fuse_insns (insn);
 
   if (may_trap_p (x))
     /* Avoid moving trapping instructions across function calls that might

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

[Kyrill Tkachov]

Ping.
https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00958.html

Kyrill

On 14/07/14 11:01, Kyrill Tkachov wrote:
> On 11/07/14 14:20, Alexander Monakov wrote:
>> On Fri, 11 Jul 2014, Kyrill Tkachov wrote:
>>> On 10/07/14 22:53, Maxim Kuvyrkov wrote:
>>>> The patch looks good to me, but some cleanup suggestions below.
>>> Thanks, here's an updated patch.
>>> How's this?
>> You need to remove 'if (targetm. ...) SCHED_GROUP_P (insn) = 1;' from the
>> first if branch, keeping only one SCHED_GROUP_P assignment at the end of the
>> function.
>>
>> Alexander
> Thanks for the pointer, I had hurried a bit.
> Here is the updated patch.
>
> Kyrill
>
> 2014-07-14  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>               Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>       * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
>       to any two insns.  Update comment.  Rename to sched_macro_fuse_insns.
>       (sched_analyze_insn): Update use of try_group_insn to
>       sched_macro_fuse_insns.
>       * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd arguments
>       that are not conditional jumps.

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

[Maxim Kuvyrkov]

On Jul 24, 2014, at 10:11 AM, Kyrill Tkachov <kyrylo.tkachov@arm.com> wrote:

> Ping.
> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00958.html
> 
> Kyrill
> 
> On 14/07/14 11:01, Kyrill Tkachov wrote:
>> On 11/07/14 14:20, Alexander Monakov wrote:
>>> On Fri, 11 Jul 2014, Kyrill Tkachov wrote:
>>>> On 10/07/14 22:53, Maxim Kuvyrkov wrote:
>>>>> The patch looks good to me, but some cleanup suggestions below.
>>>> Thanks, here's an updated patch.
>>>> How's this?
>>> You need to remove 'if (targetm. ...) SCHED_GROUP_P (insn) = 1;' from the
>>> first if branch, keeping only one SCHED_GROUP_P assignment at the end of the
>>> function.
>>> 
>>> Alexander
>> Thanks for the pointer, I had hurried a bit.
>> Here is the updated patch.
>> 

Hi Kyrill,

I have reviewed the latest version of your patch and it is perfectly fine with me.  You need to wait for an ack from the official maintainer to commit your patch.

Thank you,

--
Maxim Kuvyrkov
www.linaro.org

>> Kyrill
>> 
>> 2014-07-14  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>>              Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>> 
>>      * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
>>      to any two insns.  Update comment.  Rename to sched_macro_fuse_insns.
>>      (sched_analyze_insn): Update use of try_group_insn to
>>      sched_macro_fuse_insns.
>>      * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd arguments
>>      that are not conditional jumps.
> 
> 

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

[Jeff Law]

On 07/24/14 03:11, Kyrill Tkachov wrote:
> Ping.
> https://gcc.gnu.org/ml/gcc-patches/2014-07/msg00958.html
>
> Kyrill
>
> On 14/07/14 11:01, Kyrill Tkachov wrote:
>> On 11/07/14 14:20, Alexander Monakov wrote:
>>> On Fri, 11 Jul 2014, Kyrill Tkachov wrote:
>>>> On 10/07/14 22:53, Maxim Kuvyrkov wrote:
>>>>> The patch looks good to me, but some cleanup suggestions below.
>>>> Thanks, here's an updated patch.
>>>> How's this?
>>> You need to remove 'if (targetm. ...) SCHED_GROUP_P (insn) = 1;' from
>>> the
>>> first if branch, keeping only one SCHED_GROUP_P assignment at the end
>>> of the
>>> function.
>>>
>>> Alexander
>> Thanks for the pointer, I had hurried a bit.
>> Here is the updated patch.
>>
>> Kyrill
>>
>> 2014-07-14  Ramana Radhakrishnan <ramana.radhakrishnan@arm.com>
>>               Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>       * sched-deps.c (try_group_insn): Generalise macro fusion hook usage
>>       to any two insns.  Update comment.  Rename to
>> sched_macro_fuse_insns.
>>       (sched_analyze_insn): Update use of try_group_insn to
>>       sched_macro_fuse_insns.
>>       * config/i386/i386.c (ix86_macro_fusion_pair_p): Reject 2nd
>> arguments
>>       that are not conditional jumps.
This is fine.  Thanks for your patience.

jeff