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.tkac...@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.tkac...@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.commit a1bf782123e461726fd016d440a8d81679ee9dc0
Author: Kyrylo Tkachov <kyrylo.tkac...@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 givenenum 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)));
}
}
