diff mbox

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

Message ID 5464BBA3.1050503@arm.com
State New
Headers show

Commit Message

Kyrylo Tkachov Nov. 13, 2014, 2:09 p.m. UTC 
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.

Comments

Jeff Law Nov. 14, 2014, 5:17 a.m. UTC | #1 
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
diff mbox

Patch

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