| Message ID | 0339d94b-e63b-d4b5-ee11-04133ad2b418@bell.net |
|---|---|
| State | New |
| Headers | show |
| Series | : Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority | expand |
On 09/03/2018 08:32 AM, John David Anglin wrote: > The documentation for TARGET_SCHED_ADJUST_PRIORITY indicates that the > hook can > reduce the priority of INSN to execute it later. The hppa hook only > reduces the priority > and it has been this way for years. However, the assert in > sel_target_adjust_priority() > prevents reduction of the priority. > > The attached change revises the assert to allow the priority to be > reduced to zero. > > This fixes PR rtl-optimization/85458. > > Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11 and > hppa64-hp-hpux11.11. > > I must admit that this happens so infrequently that I have to wonder if > the hook provides > any benefit on hppa. It was supposed to keep addil instructions close > to the following instruction > to reduce pressure on register %r1. > > Okay? > > Dave > > -- > John David Anglin dave.anglin@bell.net > > > sel-sched.c.d > > > 2018-09-03 John David Anglin <danglin@gcc.gnu.org> > > PR rtl-optimization/85458 > * sel-sched.c (sel_target_adjust_priority): Allow backend adjust > priority hook to reduce the priority of EXPR. OK. And yes, this is to try and keep the addil and the next use of %r1 close to each other and hopefully avoid spilling %r1. I don't recall writing this code, but according to git blame I did :-) THe original is circa 1995. Way back then folks were really concerned about the quality of code the PA generated, particularly when accessing objects in static storage. We went to some great lengths to try and optimize that code. One of the significant issues was that in 1995, we didn't have IRA/LRA and in fact we didn't have localized spilling. So when we needed %r1 to satisfy a spill, we had to spill every pseudo that had been allocated to %r1 regardless of whether or not it conflicted with the need. The results looked truly horrific. At that time we were also consolidating all static objects within a translation unit into a structure. That allowed unrelated objects to share a single addil. This required deep copying tree nodes -- back when we still used obstacks and folks would happily stuff an integer object into a tree field. It was ultimately unmaintainable and scrapped. One of the conclusions after doing all that work was that it ultimately didn't really matter. While the code looked a hell of a lot better and was more compact, it didn't actually perform any better on the PA8000 generation hardware that was hitting the streets. There's various reasons for that, but the most important in my mind was that the addil is just a constant. So it's subject to LICM, CSE, PRE, etc. THere's just not that many from a dynamic instruction standpoint. Combine that with the capabilities of the PA8000 and how we scheduled PA8000 code and the %r1 spill avoidance and addil elimination just wasn't a real factor in code performance. Anyway, the patch is fine. Your call if you want to just kill the code in the target file, it's just not terribly important anymore. jeff
On Sep 06 2018, Jeff Law <law@redhat.com> wrote: > On 09/03/2018 08:32 AM, John David Anglin wrote: >> The documentation for TARGET_SCHED_ADJUST_PRIORITY indicates that the >> hook can >> reduce the priority of INSN to execute it later. The hppa hook only >> reduces the priority >> and it has been this way for years. However, the assert in >> sel_target_adjust_priority() >> prevents reduction of the priority. >> >> The attached change revises the assert to allow the priority to be >> reduced to zero. >> >> This fixes PR rtl-optimization/85458. >> >> Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11 and >> hppa64-hp-hpux11.11. >> >> I must admit that this happens so infrequently that I have to wonder if >> the hook provides >> any benefit on hppa. It was supposed to keep addil instructions close >> to the following instruction >> to reduce pressure on register %r1. >> >> Okay? >> >> Dave >> >> -- >> John David Anglin dave.anglin@bell.net >> >> >> sel-sched.c.d >> >> >> 2018-09-03 John David Anglin <danglin@gcc.gnu.org> >> >> PR rtl-optimization/85458 >> * sel-sched.c (sel_target_adjust_priority): Allow backend adjust >> priority hook to reduce the priority of EXPR. > OK. That breaks ia64. during RTL pass: mach /usr/local/gcc/test/gcc/testsuite/gcc.c-torture/compile/20010102-1.c: In function '_obstack_newchunk': /usr/local/gcc/test/gcc/testsuite/gcc.c-torture/compile/20010102-1.c:101:1: internal compiler error: in sel_target_adjust_priority, at sel-sched.c:3333 0x40000000010bb68f sel_target_adjust_priority ../../gcc/sel-sched.c:3333 0x40000000010bb68f fill_vec_av_set ../../gcc/sel-sched.c:3727 0x40000000010bd45f fill_ready_list ../../gcc/sel-sched.c:4028 0x40000000010bd45f find_best_expr ../../gcc/sel-sched.c:4388 0x40000000010bd45f fill_insns ../../gcc/sel-sched.c:5549 0x40000000010c29cf schedule_on_fences ../../gcc/sel-sched.c:7366 0x40000000010c29cf sel_sched_region_2 ../../gcc/sel-sched.c:7504 0x40000000010c510f sel_sched_region_1 ../../gcc/sel-sched.c:7546 0x40000000010c700f sel_sched_region(int) ../../gcc/sel-sched.c:7647 0x40000000010c9def run_selective_scheduling() ../../gcc/sel-sched.c:7733 0x40000000019e473f ia64_reorg ../../gcc/config/ia64/ia64.c:9857 0x40000000010314cf execute ../../gcc/reorg.c:3984 Andreas.
On 2018-09-10 8:35 AM, Andreas Schwab wrote: > On Sep 06 2018, Jeff Law <law@redhat.com> wrote: > >> On 09/03/2018 08:32 AM, John David Anglin wrote: >>> The documentation for TARGET_SCHED_ADJUST_PRIORITY indicates that the >>> hook can >>> reduce the priority of INSN to execute it later. The hppa hook only >>> reduces the priority >>> and it has been this way for years. However, the assert in >>> sel_target_adjust_priority() >>> prevents reduction of the priority. >>> >>> The attached change revises the assert to allow the priority to be >>> reduced to zero. >>> >>> This fixes PR rtl-optimization/85458. >>> >>> Tested on hppa-unknown-linux-gnu, hppa2.0w-hp-hpux11.11 and >>> hppa64-hp-hpux11.11. >>> >>> I must admit that this happens so infrequently that I have to wonder if >>> the hook provides >>> any benefit on hppa. It was supposed to keep addil instructions close >>> to the following instruction >>> to reduce pressure on register %r1. >>> >>> Okay? >>> >>> Dave >>> >>> -- >>> John David Anglin dave.anglin@bell.net >>> >>> >>> sel-sched.c.d >>> >>> >>> 2018-09-03 John David Anglin <danglin@gcc.gnu.org> >>> >>> PR rtl-optimization/85458 >>> * sel-sched.c (sel_target_adjust_priority): Allow backend adjust >>> priority hook to reduce the priority of EXPR. >> OK. > That breaks ia64. > > during RTL pass: mach > /usr/local/gcc/test/gcc/testsuite/gcc.c-torture/compile/20010102-1.c: In function '_obstack_newchunk': > /usr/local/gcc/test/gcc/testsuite/gcc.c-torture/compile/20010102-1.c:101:1: internal compiler error: in sel_target_adjust_priority, at sel-sched.c:3333 > 0x40000000010bb68f sel_target_adjust_priority > ../../gcc/sel-sched.c:3333 > 0x40000000010bb68f fill_vec_av_set > ../../gcc/sel-sched.c:3727 > 0x40000000010bd45f fill_ready_list > ../../gcc/sel-sched.c:4028 > 0x40000000010bd45f find_best_expr > ../../gcc/sel-sched.c:4388 > 0x40000000010bd45f fill_insns > ../../gcc/sel-sched.c:5549 > 0x40000000010c29cf schedule_on_fences > ../../gcc/sel-sched.c:7366 > 0x40000000010c29cf sel_sched_region_2 > ../../gcc/sel-sched.c:7504 > 0x40000000010c510f sel_sched_region_1 > ../../gcc/sel-sched.c:7546 > 0x40000000010c700f sel_sched_region(int) > ../../gcc/sel-sched.c:7647 > 0x40000000010c9def run_selective_scheduling() > ../../gcc/sel-sched.c:7733 > 0x40000000019e473f ia64_reorg > ../../gcc/config/ia64/ia64.c:9857 > 0x40000000010314cf execute > ../../gcc/reorg.c:3984 It looks like negative priorities occur on ia64. If that's reasonable, then the assert should be removed. On the other hand, maybe there is a bug in setting the expression priority. Dave
PR rtl-optimization/85458
* sel-sched.c (sel_target_adjust_priority): Remove wrong
assertion.
diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c
index 824f1ec340..1be977d70b 100644
--- a/gcc/sel-sched.c
+++ b/gcc/sel-sched.c
@@ -3330,8 +3330,6 @@ sel_target_adjust_priority (expr_t expr)
else
new_priority = priority;
- gcc_assert (new_priority >= 0);
-
/* If the priority has changed, adjust EXPR_PRIORITY_ADJ accordingly. */
EXPR_PRIORITY_ADJ (expr) = new_priority - EXPR_PRIORITY (expr);
On 2018-09-17 5:08 AM, Andreas Schwab wrote: > PR rtl-optimization/85458 > * sel-sched.c (sel_target_adjust_priority): Remove wrong > assertion. > > diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c > index 824f1ec340..1be977d70b 100644 > --- a/gcc/sel-sched.c > +++ b/gcc/sel-sched.c > @@ -3330,8 +3330,6 @@ sel_target_adjust_priority (expr_t expr) > else > new_priority = priority; > > - gcc_assert (new_priority >= 0); > - > /* If the priority has changed, adjust EXPR_PRIORITY_ADJ accordingly. */ > EXPR_PRIORITY_ADJ (expr) = new_priority - EXPR_PRIORITY (expr); > I added the assert because the hppa implementation of TARGET_SCHED_ADJUST_PRIORITY assumes scheduling priorities are non negative. If that is not the case, I tend to think this should be documented. It seems ia64 is the only target tripping on the assert. Dave
On Sep 17 2018, John David Anglin <dave.anglin@bell.net> wrote: > On 2018-09-17 5:08 AM, Andreas Schwab wrote: >> PR rtl-optimization/85458 >> * sel-sched.c (sel_target_adjust_priority): Remove wrong >> assertion. >> >> diff --git a/gcc/sel-sched.c b/gcc/sel-sched.c >> index 824f1ec340..1be977d70b 100644 >> --- a/gcc/sel-sched.c >> +++ b/gcc/sel-sched.c >> @@ -3330,8 +3330,6 @@ sel_target_adjust_priority (expr_t expr) >> else >> new_priority = priority; >> - gcc_assert (new_priority >= 0); >> - >> /* If the priority has changed, adjust EXPR_PRIORITY_ADJ accordingly. */ >> EXPR_PRIORITY_ADJ (expr) = new_priority - EXPR_PRIORITY (expr); >> > I added the assert because the hppa implementation of > TARGET_SCHED_ADJUST_PRIORITY assumes > scheduling priorities are non negative. If that is not the case, I tend > to think this should be documented. > > It seems ia64 is the only target tripping on the assert. The assertion only happens at -O3, see <http://gcc.gnu.org/ml/gcc-testresults/2018-09/msg01218.html>. Andreas.
On 9/17/18 3:08 AM, Andreas Schwab wrote: > PR rtl-optimization/85458 > * sel-sched.c (sel_target_adjust_priority): Remove wrong > assertion. Under what conditions is the new priority negative? Without digging deep into the ia64 port that just seems wrong. jeff
On Sep 17 2018, Jeff Law <law@redhat.com> wrote: > On 9/17/18 3:08 AM, Andreas Schwab wrote: >> PR rtl-optimization/85458 >> * sel-sched.c (sel_target_adjust_priority): Remove wrong >> assertion. > Under what conditions is the new priority negative? Without digging > deep into the ia64 port that just seems wrong. It is created in create_speculation_check: Starting program: /daten/src/gcc/c-ia64/gcc/cc1 -O3 -funroll-loops ../../gcc/gcc/testsuite/gcc.c-torture/compile/20010102-1.c _obstack_newchunk Analyzing compilation unit Performing interprocedural optimizations <*free_lang_data> <visibility> <build_ssa_passes> <opt_local_passes> <targetclone> <free-fnsummary>Streaming LTO <whole-program> <profile_estimate> <icf> <devirt> <cp> <fnsummary> <inline> <pure-const> <free-fnsummary> <static-var> <single-use> <comdats>Assembling functions: <materialize-all-clones> _obstack_newchunk Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=47, orig_insn=0x7ffff6b6b4c0) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $18 = 13 (gdb) c Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=47, orig_insn=0x7ffff6b6b680) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $19 = 11 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=47, orig_insn=0x7ffff6b6ba00) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $20 = 7 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=26, orig_insn=0x7ffff6bb1940) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $21 = 1 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253978, orig_insn=0x7ffff6b6b300) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $22 = 15 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253952, orig_insn=0x7ffff6b55240) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $23 = 0 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253999, orig_insn=0x7ffff6b554c0) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $24 = 0 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253987, orig_insn=0x7ffff6b55700) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $25 = 0 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253978, orig_insn=0x7ffff6b55940) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $26 = 0 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253978, orig_insn=0x7ffff6b55b80) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $27 = 0 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253952, orig_insn=0x7ffff6b58540) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $28 = 0 (gdb) Continuing. Breakpoint 6, create_speculation_check (c_expr=0x7fffffffd250, check_ds=253952, orig_insn=0x7ffff6b586c0) at ../../gcc/gcc/sel-sched.c:1820 1820 if (recovery_block != NULL) $29 = -1 (gdb) Continuing. Breakpoint 1, fancy_abort (file=0x13ead27 "../../gcc/gcc/sel-sched.c", line=3333, function=0x13ebe30 <sel_target_adjust_priority(_expr*)::__FUNCTION__> "sel_target_adjust_priority") at ../../gcc/gcc/diagnostic.c:1559 1559 { Andreas.
On 9/18/18 1:52 AM, Andreas Schwab wrote: > On Sep 17 2018, Jeff Law <law@redhat.com> wrote: > >> On 9/17/18 3:08 AM, Andreas Schwab wrote: >>> PR rtl-optimization/85458 >>> * sel-sched.c (sel_target_adjust_priority): Remove wrong >>> assertion. >> Under what conditions is the new priority negative? Without digging >> deep into the ia64 port that just seems wrong. > > It is created in create_speculation_check: But that says nothing about why this happens or whether or not it's a valid state. /* Decrease priority of check by difference of load/check instruction latencies. */ EXPR_PRIORITY (INSN_EXPR (insn)) -= (sel_vinsn_cost (INSN_VINSN (orig_insn)) - sel_vinsn_cost (INSN_VINSN (insn))); There's nothing inherently wrong with having one cost be higher than the other. So I don't think this is a problem with computing costs in the target files. This feels a lot more like a bug in sel-sched.c My inclination would be to declare negative priorities invalid and clamp the value in sel-sched.c. THere's other places in sel-sched.c that look fishy as well. Of course my other inclination would be to kill sel-sched completely. Jeff
Index: sel-sched.c =================================================================== --- sel-sched.c (revision 264045) +++ sel-sched.c (working copy) @@ -3330,11 +3330,11 @@ else new_priority = priority; + gcc_assert (new_priority >= 0); + /* If the priority has changed, adjust EXPR_PRIORITY_ADJ accordingly. */ EXPR_PRIORITY_ADJ (expr) = new_priority - EXPR_PRIORITY (expr); - gcc_assert (EXPR_PRIORITY_ADJ (expr) >= 0); - if (sched_verbose >= 4) sel_print ("sel_target_adjust_priority: insn %d, %d+%d = %d.\n", INSN_UID (EXPR_INSN_RTX (expr)), EXPR_PRIORITY (expr),