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

[Kyrill Tkachov <kyrylo dot tkachov at arm dot com>]

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
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" } } */