[PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns

[PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns

[Kyrill Tkachov]

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


Hi all,

I noticed that the check for modified_in_p in sched-deps is not needed 
and needlessly restricts the insn pairs that we can check for macro 
fusion in the backend hooks.
This patch removes the check. Currently that execution path is only used 
in arm and aarch64.
This enables the backend hooks to catch more cases that I'm working on.

Bootstrapped on aarch64, x86, arm. Tested on aarch64.

Ok for trunk?

Thanks,
Kyrill

2014-11-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p
     in the not condtional jump case.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sched-deps-modified.patch --]
[-- Type: text/x-patch; name=sched-deps-modified.patch, Size: 440 bytes --]

diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index 6cdb95a..b3827ba 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2877,8 +2877,7 @@ sched_macro_fuse_insns (rtx_insn *insn)
       prev = prev_nonnote_nondebug_insn (insn);
       if (!prev
           || !insn_set
-          || !single_set (prev)
-          || !modified_in_p (SET_DEST (insn_set), prev))
+          || !single_set (prev))
         return;
 
     }

Re: [PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns

[Jeff Law]

On 11/12/14 09:23, Kyrill Tkachov wrote:
>
> Hi all,
>
> I noticed that the check for modified_in_p in sched-deps is not needed
> and needlessly restricts the insn pairs that we can check for macro
> fusion in the backend hooks.
> This patch removes the check. Currently that execution path is only used
> in arm and aarch64.
> This enables the backend hooks to catch more cases that I'm working on.
>
> Bootstrapped on aarch64, x86, arm. Tested on aarch64.
>
> Ok for trunk?
>
> Thanks,
> Kyrill
>
> 2014-11-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>
>      * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p
>      in the not condtional jump case.
Typo in ChangeLog "condtional"

Can you please include a testcase which shows fusion occurring after 
this patch where it does not occur before this patch.

I think you probably need to include some kind of doc change around this 
change since you're effectively pushing responsibility for checking this 
into the ports.

With the testcase and doc change, this will probably be OK.  Please take 
care of those, repost for a review of the test & doc changes.

jeff

Re: [PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns

[Kyrill Tkachov]

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


On 12/11/14 23:18, Jeff Law wrote:
> On 11/12/14 09:23, Kyrill Tkachov wrote:
>> Hi all,
>>
>> I noticed that the check for modified_in_p in sched-deps is not needed
>> and needlessly restricts the insn pairs that we can check for macro
>> fusion in the backend hooks.
>> This patch removes the check. Currently that execution path is only used
>> in arm and aarch64.
>> This enables the backend hooks to catch more cases that I'm working on.
>>
>> Bootstrapped on aarch64, x86, arm. Tested on aarch64.
>>
>> Ok for trunk?
>>
>> Thanks,
>> Kyrill
>>
>> 2014-11-12  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>
>>
>>       * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p
>>       in the not condtional jump case.
> Typo in ChangeLog "condtional"
>
> Can you please include a testcase which shows fusion occurring after
> this patch where it does not occur before this patch.
>
> I think you probably need to include some kind of doc change around this
> change since you're effectively pushing responsibility for checking this
> into the ports.
>
> With the testcase and doc change, this will probably be OK.  Please take
> care of those, repost for a review of the test & doc changes.
>
> jeff

Hi Jeff, thanks for looking at this.

I've updated the documentation for the hook.
The testcase I was looking at involves fusing the AArch64 adrp+add 
instructions and depends on the backend implementation of the matching 
code, under review currently at 
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01263.html.

I'm attaching a reduced testcase that generates an adrp and an add and 
due to the restriction addressed in this patch it doesn't call the 
backend hook for a pair of adrp and add instructions, causing them to be 
scheduled apart.
I don't think it's a good candidate for the testsuite though because 
it's not easy to scan for consequent assembly instructions from Dejagnu 
and the instruction pair may end up scheduled together for some tuning 
parameters even though the macro fusion hook does not trigger for them 
as it should.

What do you think of this patch?

Kyrill


2014-11-13  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p
     in the not conditional jump case.
     * doc/tm.texi (TARGET_SCHED_MACRO_FUSION_PAIR_P): Update description.
     * target.def (TARGET_SCHED_MACRO_FUSION_PAIR_P): Update description.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sched-deps-modified.patch --]
[-- Type: text/x-patch; name=sched-deps-modified.patch, Size: 3209 bytes --]

commit a1bf782123e461726fd016d440a8d81679ee9dc0
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Nov 6 12:05:03 2014 +0000

    [sched-deps] remove overly strict check on macro fusion

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 33a5a97..bc7f657 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -6482,11 +6482,13 @@ cycle.  These other insns can then be taken into account properly.
 This hook is used to check whether target platform supports macro fusion.
 @end deftypefn
 
-@deftypefn {Target Hook} bool TARGET_SCHED_MACRO_FUSION_PAIR_P (rtx_insn *@var{condgen}, rtx_insn *@var{condjmp})
-This hook is used to check whether two insns could be macro fused for
-target microarchitecture. If this hook returns true for the given insn pair
-(@var{condgen} and @var{condjmp}), scheduler will put them into a sched
-group, and they will not be scheduled apart.
+@deftypefn {Target Hook} bool TARGET_SCHED_MACRO_FUSION_PAIR_P (rtx_insn *@var{prev}, rtx_insn *@var{curr})
+This hook is used to check whether two insns should be macro fused for
+a target microarchitecture. If this hook returns true for the given insn pair
+(@var{prev} and @var{curr}), the scheduler will put them into a sched
+group, and they will not be scheduled apart.  The two insns will be either
+two SET insns or a compare and a conditional jump and this hook should
+validate any dependencies needed to fuse the two insns together.
 @end deftypefn
 
 @deftypefn {Target Hook} void TARGET_SCHED_DEPENDENCIES_EVALUATION_HOOK (rtx_insn *@var{head}, rtx_insn *@var{tail})
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index a4ea836..ee534b0 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2877,8 +2877,7 @@ sched_macro_fuse_insns (rtx_insn *insn)
       prev = prev_nonnote_nondebug_insn (insn);
       if (!prev
           || !insn_set
-          || !single_set (prev)
-          || !modified_in_p (SET_DEST (insn_set), prev))
+          || !single_set (prev))
         return;
 
     }
diff --git a/gcc/target.def b/gcc/target.def
index b2fe47b..4ada2ae 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -1067,11 +1067,13 @@ DEFHOOK
 
 DEFHOOK
 (macro_fusion_pair_p,
- "This hook is used to check whether two insns could be macro fused for\n\
-target microarchitecture. If this hook returns true for the given insn pair\n\
-(@var{condgen} and @var{condjmp}), scheduler will put them into a sched\n\
-group, and they will not be scheduled apart.",
- bool, (rtx_insn *condgen, rtx_insn *condjmp), NULL)
+ "This hook is used to check whether two insns should be macro fused for\n\
+a target microarchitecture. If this hook returns true for the given insn pair\n\
+(@var{prev} and @var{curr}), the scheduler will put them into a sched\n\
+group, and they will not be scheduled apart.  The two insns will be either\n\
+two SET insns or a compare and a conditional jump and this hook should\n\
+validate any dependencies needed to fuse the two insns together.",
+ bool, (rtx_insn *prev, rtx_insn *curr), NULL)
 
 /* The following member value is a pointer to a function called
    after evaluation forward dependencies of insns in chain given

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: fusion-testcase.c --]
[-- Type: text/x-csrc; name=fusion-testcase.c, Size: 1029 bytes --]

enum reg_class { NO_REGS, AP_REG, XRF_REGS, GENERAL_REGS, AGRF_REGS,
   XGRF_REGS, ALL_REGS, LIM_REG_CLASSES };
enum rtx_code {
REG ,
  LAST_AND_UNUSED_RTX_CODE};
typedef union rtunion_def
{
  int rtint;
} rtunion;
typedef struct rtx_def
{
  unsigned int volatil : 1;
  rtunion fld[1];
} *rtx;
extern char fixed_regs[64];
extern char global_regs[64];
int
rtx_cost (x, outer_code)
     rtx x;
{
  register enum rtx_code code;
  switch (code)
    {
    case REG:
      return ! ((((x)->volatil) && ((x)->fld[0].rtint) < 64) || ((((x)->fld[0].rtint)) == 30 || (((x)->fld[0].rtint)) == 30 || (((x)->fld[0].rtint)) == 31 || (((x)->fld[0].rtint)) == 0 || ((((x)->fld[0].rtint)) >= (64) && (((x)->fld[0].rtint)) <= (((64)) + 3)) || ((((x)->fld[0].rtint)) < 64 && ((((x)->fld[0].rtint)) == 30 || (((x)->fld[0].rtint)) == 30 || fixed_regs[((x)->fld[0].rtint)] || global_regs[((x)->fld[0].rtint)]) && ((((x)->fld[0].rtint)) ? ((((x)->fld[0].rtint) < 32) ? GENERAL_REGS : XRF_REGS) : AP_REG) != NO_REGS)));
    }
}

Re: [PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns

[Jeff Law]

On 11/13/14 07:09, Kyrill Tkachov wrote:

>
> I've updated the documentation for the hook.
> The testcase I was looking at involves fusing the AArch64 adrp+add
> instructions and depends on the backend implementation of the matching
> code, under review currently at
> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01263.html.
>
> I'm attaching a reduced testcase that generates an adrp and an add and
> due to the restriction addressed in this patch it doesn't call the
> backend hook for a pair of adrp and add instructions, causing them to be
> scheduled apart.
> I don't think it's a good candidate for the testsuite though because
> it's not easy to scan for consequent assembly instructions from Dejagnu
> and the instruction pair may end up scheduled together for some tuning
> parameters even though the macro fusion hook does not trigger for them
> as it should.
It's painful, but certainly possible to check for consecutive assembly 
instructions.  You just have to match the first instruction, its 
operands, a newline, then the 2nd instruction & operands.  The 
difficulty is in getting all the escape sequences right!

There are some examples of this.  For example mips/umips-branch-1.c

/* { dg-final { scan-assembler "\tjr?\t\\\$31\n\tmove\t\\\$2,\\\$0" } } */


Which looks for a jr instruction, some operands, then a move instruction 
on the next line.

As for instability, that's an inherent problem in some of this kind of 
stuff.  Just run it for the target you care about, possibly giving 
explicit tuning parameters that are known to make it trigger.

OK with a testcase.

jeff

Re: [PATCH][sched-deps] Remove needless check for modified_in_p when trying to fuse two non-conditional jump insns

[Kyrill Tkachov]

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


On 14/11/14 05:17, Jeff Law wrote:
> On 11/13/14 07:09, Kyrill Tkachov wrote:
>
>> I've updated the documentation for the hook.
>> The testcase I was looking at involves fusing the AArch64 adrp+add
>> instructions and depends on the backend implementation of the matching
>> code, under review currently at
>> https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01263.html.
>>
>> I'm attaching a reduced testcase that generates an adrp and an add and
>> due to the restriction addressed in this patch it doesn't call the
>> backend hook for a pair of adrp and add instructions, causing them to be
>> scheduled apart.
>> I don't think it's a good candidate for the testsuite though because
>> it's not easy to scan for consequent assembly instructions from Dejagnu
>> and the instruction pair may end up scheduled together for some tuning
>> parameters even though the macro fusion hook does not trigger for them
>> as it should.
> It's painful, but certainly possible to check for consecutive assembly
> instructions.  You just have to match the first instruction, its
> operands, a newline, then the 2nd instruction & operands.  The
> difficulty is in getting all the escape sequences right!
>
> There are some examples of this.  For example mips/umips-branch-1.c
>
> /* { dg-final { scan-assembler "\tjr?\t\\\$31\n\tmove\t\\\$2,\\\$0" } } */
>
>
> Which looks for a jr instruction, some operands, then a move instruction
> on the next line.
>
> As for instability, that's an inherent problem in some of this kind of
> stuff.  Just run it for the target you care about, possibly giving
> explicit tuning parameters that are known to make it trigger.
>
> OK with a testcase.
Hi Jeff,

I did manage to integrate it (hopefully it doesn't become fragile).
I'll commit the attached patch after the prerequisite aarch64 fusion 
patch goes in.

Thanks again,
Kyrill


2014-11-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * sched-deps.c (sched_macro_fuse_insns): Do not check modified_in_p
     in the not conditional jump case.
     * doc/tm.texi (TARGET_SCHED_MACRO_FUSION_PAIR_P): Update description.
     * target.def (TARGET_SCHED_MACRO_FUSION_PAIR_P): Update description.

2014-11-14  Kyrylo Tkachov  <kyrylo.tkachov@arm.com>

     * gcc.target/aarch64/fuse_adrp_add_1.c: New test.

>
> jeff
>
>

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: sched-deps-modified.patch --]
[-- Type: text/x-patch; name=sched-deps-modified.patch, Size: 4959 bytes --]

commit 399f71dca4f7c3d678b8f986319841b7da0ce86f
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date:   Thu Nov 6 12:05:03 2014 +0000

    [sched-deps] remove overly strict check on macro fusion

diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 8d137f5..762ffff 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -6484,11 +6484,13 @@ cycle.  These other insns can then be taken into account properly.
 This hook is used to check whether target platform supports macro fusion.
 @end deftypefn
 
-@deftypefn {Target Hook} bool TARGET_SCHED_MACRO_FUSION_PAIR_P (rtx_insn *@var{condgen}, rtx_insn *@var{condjmp})
-This hook is used to check whether two insns could be macro fused for
-target microarchitecture. If this hook returns true for the given insn pair
-(@var{condgen} and @var{condjmp}), scheduler will put them into a sched
-group, and they will not be scheduled apart.
+@deftypefn {Target Hook} bool TARGET_SCHED_MACRO_FUSION_PAIR_P (rtx_insn *@var{prev}, rtx_insn *@var{curr})
+This hook is used to check whether two insns should be macro fused for
+a target microarchitecture. If this hook returns true for the given insn pair
+(@var{prev} and @var{curr}), the scheduler will put them into a sched
+group, and they will not be scheduled apart.  The two insns will be either
+two SET insns or a compare and a conditional jump and this hook should
+validate any dependencies needed to fuse the two insns together.
 @end deftypefn
 
 @deftypefn {Target Hook} void TARGET_SCHED_DEPENDENCIES_EVALUATION_HOOK (rtx_insn *@var{head}, rtx_insn *@var{tail})
diff --git a/gcc/sched-deps.c b/gcc/sched-deps.c
index a4ea836..ee534b0 100644
--- a/gcc/sched-deps.c
+++ b/gcc/sched-deps.c
@@ -2877,8 +2877,7 @@ sched_macro_fuse_insns (rtx_insn *insn)
       prev = prev_nonnote_nondebug_insn (insn);
       if (!prev
           || !insn_set
-          || !single_set (prev)
-          || !modified_in_p (SET_DEST (insn_set), prev))
+          || !single_set (prev))
         return;
 
     }
diff --git a/gcc/target.def b/gcc/target.def
index c329b2a..0732a90 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -1067,11 +1067,13 @@ DEFHOOK
 
 DEFHOOK
 (macro_fusion_pair_p,
- "This hook is used to check whether two insns could be macro fused for\n\
-target microarchitecture. If this hook returns true for the given insn pair\n\
-(@var{condgen} and @var{condjmp}), scheduler will put them into a sched\n\
-group, and they will not be scheduled apart.",
- bool, (rtx_insn *condgen, rtx_insn *condjmp), NULL)
+ "This hook is used to check whether two insns should be macro fused for\n\
+a target microarchitecture. If this hook returns true for the given insn pair\n\
+(@var{prev} and @var{curr}), the scheduler will put them into a sched\n\
+group, and they will not be scheduled apart.  The two insns will be either\n\
+two SET insns or a compare and a conditional jump and this hook should\n\
+validate any dependencies needed to fuse the two insns together.",
+ bool, (rtx_insn *prev, rtx_insn *curr), NULL)
 
 /* The following member value is a pointer to a function called
    after evaluation forward dependencies of insns in chain given
diff --git a/gcc/testsuite/gcc.target/aarch64/fuse_adrp_add_1.c b/gcc/testsuite/gcc.target/aarch64/fuse_adrp_add_1.c
new file mode 100644
index 0000000..074c629
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/fuse_adrp_add_1.c
@@ -0,0 +1,45 @@
+/* { dg-do compile } */
+/* { dg-options "-O3 -mcpu=cortex-a57" } */
+
+enum reg_class { NO_REGS, AP_REG, XRF_REGS, GENERAL_REGS, AGRF_REGS,
+                 XGRF_REGS, ALL_REGS, LIM_REG_CLASSES };
+
+enum rtx_code { REG,  LAST_AND_UNUSED_RTX_CODE };
+
+typedef union rtunion_def
+{
+  int rtint;
+} rtunion;
+
+typedef struct rtx_def
+{
+  unsigned int volatil : 1;
+  rtunion fld[1];
+} *rtx;
+
+extern char fixed_regs[64];
+extern char global_regs[64];
+
+int
+rtx_cost (rtx x, int outer_code)
+{
+  register enum rtx_code code;
+  switch (code)
+    {
+      case REG:
+        return ! ((((x)->volatil) && ((x)->fld[0].rtint) < 64)
+                 || ((((x)->fld[0].rtint)) == 30 || (((x)->fld[0].rtint)) == 30
+                 || (((x)->fld[0].rtint)) == 31 || (((x)->fld[0].rtint)) == 0
+                 || ((((x)->fld[0].rtint)) >= (64)
+                     && (((x)->fld[0].rtint)) <= (((64)) + 3))
+                 || ((((x)->fld[0].rtint)) < 64 && ((((x)->fld[0].rtint)) == 30
+                 || (((x)->fld[0].rtint)) == 30 || fixed_regs[((x)->fld[0].rtint)]
+                 || global_regs[((x)->fld[0].rtint)])
+                    && ((((x)->fld[0].rtint))
+                          ? ((((x)->fld[0].rtint) < 32)
+                             ? GENERAL_REGS : XRF_REGS)
+                          : AP_REG) != NO_REGS)));
+    }
+}
+
+/* { dg-final { scan-assembler "adrp\tx.*, fixed_regs\n\tadd\tx.*, x.*fixed_regs" } } */