| Message ID | CAFULd4ZFo0R5Em=_3O4AhEC-pg=-rgiTv01=5rLwRPgjMCiWBQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Improve splitX passes management | expand |
On Thu, Feb 06, 2020 at 12:13:35PM +0100, Uros Bizjak wrote: > The names of split_before_sched2 ("split4") and split_before_regstack > ("split3") do not reflect their insertion point in the sequence of passes, > where split_before_regstack follows split_before_sched2. Reorder the code > and rename the passes to reflect the reality. Renaming them to other splitN doesn't help much :-/ Having stable names is more important (some archs actually use these names), I'd say. But it's hard to come up with shortish more meaningful names. There is no real need for the N in splitN to be in order, but sure it can be surprising otherwise. > +bool > +pass_split_before_regstack::gate (function *) > +{ > +#if HAVE_ATTR_length && defined (STACK_REGS) > + /* If flow2 creates new instructions which need splitting > + and scheduling after reload is not done, they might not be > + split until final which doesn't allow splitting > + if HAVE_ATTR_length. */ > + return !enable_split_before_sched2 (); > +#else > + return false; > +#endif > +} flow.c was deleted in 2006... Is this split still needed at all? If so, change the comment please? :-) Segher
On Fri, Feb 7, 2020 at 5:41 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Thu, Feb 06, 2020 at 12:13:35PM +0100, Uros Bizjak wrote: > > The names of split_before_sched2 ("split4") and split_before_regstack > > ("split3") do not reflect their insertion point in the sequence of passes, > > where split_before_regstack follows split_before_sched2. Reorder the code > > and rename the passes to reflect the reality. > > Renaming them to other splitN doesn't help much :-/ Having stable names > is more important (some archs actually use these names), I'd say. But True, this is why I took care to found and fix all references to existing names. It is a simple substitution of old name with a new. > it's hard to come up with shortish more meaningful names. > > There is no real need for the N in splitN to be in order, but sure it > can be surprising otherwise. I'm not familiar with tree passes, how the situation is handled there. If a new instance of the same pass is inserted before existing pass, does name of the existing pass get changed? > > +bool > > +pass_split_before_regstack::gate (function *) > > +{ > > +#if HAVE_ATTR_length && defined (STACK_REGS) > > + /* If flow2 creates new instructions which need splitting > > + and scheduling after reload is not done, they might not be > > + split until final which doesn't allow splitting > > + if HAVE_ATTR_length. */ > > + return !enable_split_before_sched2 (); > > +#else > > + return false; > > +#endif > > +} > > flow.c was deleted in 2006... Is this split still needed at all? If > so, change the comment please? :-) Hm, I don't know in general, but x86 needs a late split at the point where instantiated registers won't change in the insn. Mainly an optimization, not correctness issue, but the split is needed to fix some HW issues in the processor (please see comments in i386.md around insn condition involving epilogue_completed flag. As you suggested in the other mail, the issue with many late split passes (passes 3-5) should be rewiewed, considering also other (e.g. non-stack regs) targets. Uros.
Hi again, On Sat, Feb 08, 2020 at 11:54:48AM +0100, Uros Bizjak wrote: > On Fri, Feb 7, 2020 at 5:41 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > On Thu, Feb 06, 2020 at 12:13:35PM +0100, Uros Bizjak wrote: > > > The names of split_before_sched2 ("split4") and split_before_regstack > > > ("split3") do not reflect their insertion point in the sequence of passes, > > > where split_before_regstack follows split_before_sched2. Reorder the code > > > and rename the passes to reflect the reality. > > > > Renaming them to other splitN doesn't help much :-/ Having stable names > > is more important (some archs actually use these names), I'd say. But > > True, this is why I took care to found and fix all references to > existing names. It is a simple substitution of old name with a new. Yeah, and reading the patch is a bit challenging ;-) If we had real names here, this would be simpler. Something for the future. > I'm not familiar with tree passes, how the situation is handled there. > If a new instance of the same pass is inserted before existing pass, > does name of the existing pass get changed? I don't know either. > > > +bool > > > +pass_split_before_regstack::gate (function *) > > > +{ > > > +#if HAVE_ATTR_length && defined (STACK_REGS) > > > + /* If flow2 creates new instructions which need splitting > > > + and scheduling after reload is not done, they might not be > > > + split until final which doesn't allow splitting > > > + if HAVE_ATTR_length. */ > > > + return !enable_split_before_sched2 (); > > > +#else > > > + return false; > > > +#endif > > > +} > > > > flow.c was deleted in 2006... Is this split still needed at all? If > > so, change the comment please? :-) > > Hm, I don't know in general, but x86 needs a late split at the point > where instantiated registers won't change in the insn. Right, but the explanation given in the comment is very out of date. > As you suggested in the other mail, the issue with many late split > passes (passes 3-5) should be rewiewed, considering also other (e.g. > non-stack regs) targets. Yeah. But that is not a stage 4 thing. Your patch is safe for stage 4 as far as I can see. Segher
diff --git a/gcc/config/nds32/nds32.c b/gcc/config/nds32/nds32.c index 625fa8ce7db8..acf13715d830 100644 --- a/gcc/config/nds32/nds32.c +++ b/gcc/config/nds32/nds32.c @@ -5496,7 +5496,7 @@ nds32_split_double_word_load_store_p(rtx *operands, bool load_p) return false; const char *pass_name = current_pass->name; - if (pass_name && ((strcmp (pass_name, "split4") == 0) + if (pass_name && ((strcmp (pass_name, "split3") == 0) || (strcmp (pass_name, "split5") == 0))) return !satisfies_constraint_Da (mem) || MEM_VOLATILE_P (mem); diff --git a/gcc/config/sh/sh.c b/gcc/config/sh/sh.c index 3439f1663830..a178cfd3b9c9 100644 --- a/gcc/config/sh/sh.c +++ b/gcc/config/sh/sh.c @@ -800,7 +800,7 @@ register_sh_passes (void) /* Run sh_treg_combine pass after register allocation and basic block reordering as this sometimes creates new opportunities. */ register_pass (make_pass_sh_treg_combine (g, true, "sh_treg_combine3"), - PASS_POS_INSERT_AFTER, "split4", 1); + PASS_POS_INSERT_AFTER, "split3", 1); /* Optimize sett and clrt insns, by e.g. removing them if the T bit value is known after a conditional branch. diff --git a/gcc/recog.c b/gcc/recog.c index 5790a58a9114..8c098cf5b0fe 100644 --- a/gcc/recog.c +++ b/gcc/recog.c @@ -3943,9 +3943,19 @@ make_pass_split_after_reload (gcc::context *ctxt) return new pass_split_after_reload (ctxt); } +static bool +enable_split_before_sched2 (void) +{ +#ifdef INSN_SCHEDULING + return optimize > 0 && flag_schedule_insns_after_reload; +#else + return false; +#endif +} + namespace { -const pass_data pass_data_split_before_regstack = +const pass_data pass_data_split_before_sched2 = { RTL_PASS, /* type */ "split3", /* name */ @@ -3958,61 +3968,38 @@ const pass_data pass_data_split_before_regstack = 0, /* todo_flags_finish */ }; -class pass_split_before_regstack : public rtl_opt_pass +class pass_split_before_sched2 : public rtl_opt_pass { public: - pass_split_before_regstack (gcc::context *ctxt) - : rtl_opt_pass (pass_data_split_before_regstack, ctxt) + pass_split_before_sched2 (gcc::context *ctxt) + : rtl_opt_pass (pass_data_split_before_sched2, ctxt) {} /* opt_pass methods: */ - virtual bool gate (function *); + virtual bool gate (function *) + { + return enable_split_before_sched2 (); + } + virtual unsigned int execute (function *) { split_all_insns (); return 0; } -}; // class pass_split_before_regstack - -bool -pass_split_before_regstack::gate (function *) -{ -#if HAVE_ATTR_length && defined (STACK_REGS) - /* If flow2 creates new instructions which need splitting - and scheduling after reload is not done, they might not be - split until final which doesn't allow splitting - if HAVE_ATTR_length. */ -# ifdef INSN_SCHEDULING - return !optimize || !flag_schedule_insns_after_reload; -# else - return true; -# endif -#else - return false; -#endif -} +}; // class pass_split_before_sched2 } // anon namespace rtl_opt_pass * -make_pass_split_before_regstack (gcc::context *ctxt) -{ - return new pass_split_before_regstack (ctxt); -} - -static unsigned int -rest_of_handle_split_before_sched2 (void) +make_pass_split_before_sched2 (gcc::context *ctxt) { -#ifdef INSN_SCHEDULING - split_all_insns (); -#endif - return 0; + return new pass_split_before_sched2 (ctxt); } namespace { -const pass_data pass_data_split_before_sched2 = +const pass_data pass_data_split_before_regstack = { RTL_PASS, /* type */ "split4", /* name */ @@ -4025,36 +4012,43 @@ const pass_data pass_data_split_before_sched2 = 0, /* todo_flags_finish */ }; -class pass_split_before_sched2 : public rtl_opt_pass +class pass_split_before_regstack : public rtl_opt_pass { public: - pass_split_before_sched2 (gcc::context *ctxt) - : rtl_opt_pass (pass_data_split_before_sched2, ctxt) + pass_split_before_regstack (gcc::context *ctxt) + : rtl_opt_pass (pass_data_split_before_regstack, ctxt) {} /* opt_pass methods: */ - virtual bool gate (function *) - { -#ifdef INSN_SCHEDULING - return optimize > 0 && flag_schedule_insns_after_reload; -#else - return false; -#endif - } - + virtual bool gate (function *); virtual unsigned int execute (function *) { - return rest_of_handle_split_before_sched2 (); + split_all_insns (); + return 0; } -}; // class pass_split_before_sched2 +}; // class pass_split_before_regstack + +bool +pass_split_before_regstack::gate (function *) +{ +#if HAVE_ATTR_length && defined (STACK_REGS) + /* If flow2 creates new instructions which need splitting + and scheduling after reload is not done, they might not be + split until final which doesn't allow splitting + if HAVE_ATTR_length. */ + return !enable_split_before_sched2 (); +#else + return false; +#endif +} } // anon namespace rtl_opt_pass * -make_pass_split_before_sched2 (gcc::context *ctxt) +make_pass_split_before_regstack (gcc::context *ctxt) { - return new pass_split_before_sched2 (ctxt); + return new pass_split_before_regstack (ctxt); } namespace {
The names of split_before_sched2 ("split4") and split_before_regstack ("split3") do not reflect their insertion point in the sequence of passes, where split_before_regstack follows split_before_sched2. Reorder the code and rename the passes to reflect the reality. split_before_regstack pass does not need to run if split_before_sched2 pass was already performed. Introduce enable_split_before_sched2 function to simplify gating functions of these two passes. There is no need for a separate rest_of_handle_split_before_sched2. split_all_insns can be called unconditionally from pass_split_before_sched2::execute, since the corresponding gating function determines if the pass is executed or not. Bootstrapped and regression tested on x86_64-linux-gnu {,-m32}. 2020-02-06 Uroš Bizjak <ubizjak@gmail.com> * recog.c: Move pass_split_before_sched2 code in front of pass_split_before_regstack. (pass_data_split_before_sched2): Rename pass to split3 from split4. (pass_data_split_before_regstack): Rename pass to split4 from split3. (rest_of_handle_split_before_sched2): Remove. (pass_split_before_sched2::execute): Unconditionally call split_all_insns. (enable_split_before_sched2): New function. (pass_split_before_sched2::gate): Use enable_split_before_sched2. (pass_split_before_regstack::gate): Ditto. * config/nds32/nds32.c (nds32_split_double_word_load_store_p): Update name check for renamed split4 pass. * config/sh/sh.c (register_sh_passes): Update pass insertion point for renamed split4 pass. Uros.