| Message ID | f4caf2d278d3cd556523b068c99bcdd737bfca87.1486149867.git.segher@kernel.crashing.org |
|---|---|
| State | New |
| Headers | show |
On 02/03/2017 01:12 PM, Segher Boessenkool wrote: > Scheduling should never move very expensive instructions to places they > are executed more frequently. This patch fixes that, reducing the > execution time of c-ray by over 40% (I tested on a BE Power7 system). > > This introduces a new target hook sched.can_speculate_insn which returns > whether the scheduler is allowed to speculate a given instruction. The > rs6000 implementation disallows all divide and square root instructions. > > Bootstrapped and tested on powerpc64-linux {-m32,-m64}. Is this okay > for trunk? > > > Segher > > > 2017-02-03 Segher Boessenkool <segher@kernel.crashing.org> > > PR rtl-optimization/68664 > * target.def (can_speculate_insn): New hook. > * doc/tm.texi.in (TARGET_SCHED_CAN_SPECULATE_INSN): New hook. > * doc/tm.texi: Regenerate. > * sched-rgn.c (can_schedule_ready_p): Use the new hook. > * config/rs6000/rs6000.c (TARGET_SCHED_CAN_SPECULATE_INSN): New macro. > (rs6000_sched_can_speculate_insn): New function. > > --- > gcc/config/rs6000/rs6000.c | 20 ++++++++++++++++++++ > gcc/doc/tm.texi | 6 ++++++ > gcc/doc/tm.texi.in | 2 ++ > gcc/sched-rgn.c | 19 +++++++++++++------ > gcc/target.def | 7 +++++++ > 5 files changed, 48 insertions(+), 6 deletions(-) > > diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi > index 909589c..1963402 100644 > --- a/gcc/doc/tm.texi > +++ b/gcc/doc/tm.texi > @@ -7000,6 +7000,12 @@ The structure *@var{spec_info} should be filled in by the target. > The structure describes speculation types that can be used in the scheduler. > @end deftypefn > > +@deftypefn {Target Hook} bool TARGET_SCHED_CAN_SPECULATE_INSN (rtx_insn *@var{insn}) > +Some instructions should never be speculated by the schedulers, usually > + because the instruction is too expensive to get this wrong. This hook > + should return @code{false} if @var{insn} should not be speculated. > +@end deftypefn Consider adding something like this: Define this hook to return false for instructions which are not fully modeled by the pipeline description to avoid DFA size explosion. Otherwise the scheduler may erroneously speculate those instructions into a pipeline bubble that is too small which may severely impact performance. > + > @deftypefn {Target Hook} int TARGET_SCHED_SMS_RES_MII (struct ddg *@var{g}) > This hook is called by the swing modulo scheduler to calculate a > resource-based lower bound which is based on the resources available in > diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in > index ea74d37..756c118 100644 > --- a/gcc/doc/tm.texi.in > +++ b/gcc/doc/tm.texi.in > @@ -4882,6 +4882,8 @@ them: try the first ones in this list first. > descr > @hook TARGET_SCHED_SET_SCHED_FLAGS > > +@hook TARGET_SCHED_CAN_SPECULATE_INSN > + > @hook TARGET_SCHED_SMS_RES_MII > > @hook TARGET_SCHED_DISPATCH > diff --git a/gcc/sched-rgn.c b/gcc/sched-rgn.c > index 2af3a03..a09fc5d 100644 > --- a/gcc/sched-rgn.c > +++ b/gcc/sched-rgn.c > @@ -2147,12 +2147,19 @@ static int > can_schedule_ready_p (rtx_insn *insn) > { > /* An interblock motion? */ > - if (INSN_BB (insn) != target_bb > - && IS_SPECULATIVE_INSN (insn) > - && !check_live (insn, INSN_BB (insn))) > - return 0; > - else > - return 1; > + if (INSN_BB (insn) != target_bb && IS_SPECULATIVE_INSN (insn)) > + { > + /* Cannot schedule this insn unless all operands are live. */ > + if (!check_live (insn, INSN_BB (insn))) > + return 0; > + > + /* Should not move expensive instructions speculatively. */ > + if (GET_CODE (PATTERN (insn)) != CLOBBER > + && !targetm.sched.can_speculate_insn (insn)) > + return 0; > + } You probably want to filter both CLOBBER and USE insns. OK with those two nits addressed. jeff
Hi Jeff, On Fri, Feb 03, 2017 at 04:31:58PM -0700, Jeff Law wrote: > >+@deftypefn {Target Hook} bool TARGET_SCHED_CAN_SPECULATE_INSN (rtx_insn > >*@var{insn}) > >+Some instructions should never be speculated by the schedulers, usually > >+ because the instruction is too expensive to get this wrong. This hook > >+ should return @code{false} if @var{insn} should not be speculated. > >+@end deftypefn > Consider adding something like this: > > Define this hook to return false for instructions which are not fully > modeled by the pipeline description to avoid DFA size explosion. > Otherwise the scheduler may erroneously speculate those instructions > into a pipeline bubble that is too small which may severely impact > performance. Well, it speculates it even _if_ you correctly model it, currently anyway. But I'll write something similar, good idea. > >+ if (INSN_BB (insn) != target_bb && IS_SPECULATIVE_INSN (insn)) > >+ { > >+ /* Cannot schedule this insn unless all operands are live. */ > >+ if (!check_live (insn, INSN_BB (insn))) > >+ return 0; > >+ > >+ /* Should not move expensive instructions speculatively. */ > >+ if (GET_CODE (PATTERN (insn)) != CLOBBER > >+ && !targetm.sched.can_speculate_insn (insn)) > >+ return 0; > >+ } > You probably want to filter both CLOBBER and USE insns. I had that originally (and more checks), but only CLOBBER can be speculated and there is that IS_SPECULATIVE_INSN check right before. I had the CLOBBER check in the rs6000 hook first -- get_attr_type will die on it -- but I figured it makes more sense in the generic code. > OK with those two nits addressed. Thanks! Segher
On Fri, Feb 03, 2017 at 06:28:21PM -0600, Segher Boessenkool wrote: > On Fri, Feb 03, 2017 at 04:31:58PM -0700, Jeff Law wrote: > > >+@deftypefn {Target Hook} bool TARGET_SCHED_CAN_SPECULATE_INSN (rtx_insn > > >*@var{insn}) > > >+Some instructions should never be speculated by the schedulers, usually > > >+ because the instruction is too expensive to get this wrong. This hook > > >+ should return @code{false} if @var{insn} should not be speculated. > > >+@end deftypefn > > Consider adding something like this: > > > > Define this hook to return false for instructions which are not fully > > modeled by the pipeline description to avoid DFA size explosion. > > Otherwise the scheduler may erroneously speculate those instructions > > into a pipeline bubble that is too small which may severely impact > > performance. > > Well, it speculates it even _if_ you correctly model it, currently > anyway. But I'll write something similar, good idea. This is what I committed: +DEFHOOK +(can_speculate_insn, + "Some instructions should never be speculated by the schedulers, usually\n\ + because the instruction is too expensive to get this wrong. Often such\n\ + instructions have long latency, and often they are not fully modeled in the\n\ + pipeline descriptions. This hook should return @code{false} if @var{insn}\n\ + should not be speculated.", + bool, (rtx_insn *insn), hook_bool_rtx_insn_true) Segher
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c index 98e5f91..5e85858 100644 --- a/gcc/config/rs6000/rs6000.c +++ b/gcc/config/rs6000/rs6000.c @@ -1608,6 +1608,9 @@ static const struct attribute_spec rs6000_attribute_table[] = #undef TARGET_SCHED_FREE_SCHED_CONTEXT #define TARGET_SCHED_FREE_SCHED_CONTEXT rs6000_free_sched_context +#undef TARGET_SCHED_CAN_SPECULATE_INSN +#define TARGET_SCHED_CAN_SPECULATE_INSN rs6000_sched_can_speculate_insn + #undef TARGET_VECTORIZE_BUILTIN_MASK_FOR_LOAD #define TARGET_VECTORIZE_BUILTIN_MASK_FOR_LOAD rs6000_builtin_mask_for_load #undef TARGET_VECTORIZE_SUPPORT_VECTOR_MISALIGNMENT @@ -34986,6 +34989,23 @@ rs6000_free_sched_context (void *_sc) free (_sc); } +static bool +rs6000_sched_can_speculate_insn (rtx_insn *insn) +{ + switch (get_attr_type (insn)) + { + case TYPE_DIV: + case TYPE_SDIV: + case TYPE_DDIV: + case TYPE_VECDIV: + case TYPE_SSQRT: + case TYPE_DSQRT: + return false; + + default: + return true; + } +} /* Length in units of the trampoline for entering a nested function. */ diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi index 909589c..1963402 100644 --- a/gcc/doc/tm.texi +++ b/gcc/doc/tm.texi @@ -7000,6 +7000,12 @@ The structure *@var{spec_info} should be filled in by the target. The structure describes speculation types that can be used in the scheduler. @end deftypefn +@deftypefn {Target Hook} bool TARGET_SCHED_CAN_SPECULATE_INSN (rtx_insn *@var{insn}) +Some instructions should never be speculated by the schedulers, usually + because the instruction is too expensive to get this wrong. This hook + should return @code{false} if @var{insn} should not be speculated. +@end deftypefn + @deftypefn {Target Hook} int TARGET_SCHED_SMS_RES_MII (struct ddg *@var{g}) This hook is called by the swing modulo scheduler to calculate a resource-based lower bound which is based on the resources available in diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in index ea74d37..756c118 100644 --- a/gcc/doc/tm.texi.in +++ b/gcc/doc/tm.texi.in @@ -4882,6 +4882,8 @@ them: try the first ones in this list first. @hook TARGET_SCHED_SET_SCHED_FLAGS +@hook TARGET_SCHED_CAN_SPECULATE_INSN + @hook TARGET_SCHED_SMS_RES_MII @hook TARGET_SCHED_DISPATCH diff --git a/gcc/sched-rgn.c b/gcc/sched-rgn.c index 2af3a03..a09fc5d 100644 --- a/gcc/sched-rgn.c +++ b/gcc/sched-rgn.c @@ -2147,12 +2147,19 @@ static int can_schedule_ready_p (rtx_insn *insn) { /* An interblock motion? */ - if (INSN_BB (insn) != target_bb - && IS_SPECULATIVE_INSN (insn) - && !check_live (insn, INSN_BB (insn))) - return 0; - else - return 1; + if (INSN_BB (insn) != target_bb && IS_SPECULATIVE_INSN (insn)) + { + /* Cannot schedule this insn unless all operands are live. */ + if (!check_live (insn, INSN_BB (insn))) + return 0; + + /* Should not move expensive instructions speculatively. */ + if (GET_CODE (PATTERN (insn)) != CLOBBER + && !targetm.sched.can_speculate_insn (insn)) + return 0; + } + + return 1; } /* Updates counter and other information. Split from can_schedule_ready_p () diff --git a/gcc/target.def b/gcc/target.def index 7308da1..dbb5d76 100644 --- a/gcc/target.def +++ b/gcc/target.def @@ -1480,6 +1480,13 @@ DEFHOOK_UNDOC "Return speculation types that are checked for instruction @var{insn}", unsigned int, (rtx_insn *insn), NULL) +DEFHOOK +(can_speculate_insn, + "Some instructions should never be speculated by the schedulers, usually\n\ + because the instruction is too expensive to get this wrong. This hook\n\ + should return @code{false} if @var{insn} should not be speculated.", + bool, (rtx_insn *insn), hook_bool_rtx_insn_true) + DEFHOOK_UNDOC (skip_rtx_p, "Return bool if rtx scanning should just skip current layer and\
Scheduling should never move very expensive instructions to places they are executed more frequently. This patch fixes that, reducing the execution time of c-ray by over 40% (I tested on a BE Power7 system). This introduces a new target hook sched.can_speculate_insn which returns whether the scheduler is allowed to speculate a given instruction. The rs6000 implementation disallows all divide and square root instructions. Bootstrapped and tested on powerpc64-linux {-m32,-m64}. Is this okay for trunk? Segher 2017-02-03 Segher Boessenkool <segher@kernel.crashing.org> PR rtl-optimization/68664 * target.def (can_speculate_insn): New hook. * doc/tm.texi.in (TARGET_SCHED_CAN_SPECULATE_INSN): New hook. * doc/tm.texi: Regenerate. * sched-rgn.c (can_schedule_ready_p): Use the new hook. * config/rs6000/rs6000.c (TARGET_SCHED_CAN_SPECULATE_INSN): New macro. (rs6000_sched_can_speculate_insn): New function. --- gcc/config/rs6000/rs6000.c | 20 ++++++++++++++++++++ gcc/doc/tm.texi | 6 ++++++ gcc/doc/tm.texi.in | 2 ++ gcc/sched-rgn.c | 19 +++++++++++++------ gcc/target.def | 7 +++++++ 5 files changed, 48 insertions(+), 6 deletions(-)