On Jul 10, 2014, at 8:00 PM, Kyrill Tkachov <kyrylo.tkac...@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.tkac...@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
