: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority

Message ID 0339d94b-e63b-d4b5-ee11-04133ad2b418@bell.net
State New
Headers show
Series
  • : Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
Related show

Commit Message

John David Anglin Sept. 3, 2018, 2:32 p.m.
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
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.

Comments

Jeff Law Sept. 6, 2018, 5:05 p.m. | #1
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
Andreas Schwab Sept. 10, 2018, 12:35 p.m. | #2
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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
John David Anglin Sept. 10, 2018, 2:10 p.m. | #3
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

-- 
John David Anglin  dave.anglin@bell.net
Andreas Schwab Sept. 17, 2018, 9:08 a.m. | #4
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);
 
-- 
2.19.0


Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
John David Anglin Sept. 17, 2018, 2:06 p.m. | #5
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

-- 
John David Anglin  dave.anglin@bell.net
Andreas Schwab Sept. 17, 2018, 2:19 p.m. | #6
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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Jeff Law Sept. 17, 2018, 7:30 p.m. | #7
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
Andreas Schwab Sept. 18, 2018, 7:52 a.m. | #8
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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
Jeff Law Sept. 18, 2018, 1:50 p.m. | #9
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

Patch

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),