public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
* [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
@ 2018-09-03 14:32 John David Anglin
  2018-09-06 17:05 ` Jeff Law
  0 siblings, 1 reply; 11+ messages in thread
From: John David Anglin @ 2018-09-03 14:32 UTC (permalink / raw)
  To: GCC Patches; +Cc: Jeff Law

[-- Attachment #1: Type: text/plain, Size: 802 bytes --]

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


[-- Attachment #2: sel-sched.c.d --]
[-- Type: text/plain, Size: 805 bytes --]

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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-03 14:32 [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority John David Anglin
@ 2018-09-06 17:05 ` Jeff Law
  2018-09-10 12:35   ` Andreas Schwab
  2018-09-20  1:51   ` [PATCH] hppa: Remove TARGET_SCHED_ADJUST_PRIORITY hook John David Anglin
  0 siblings, 2 replies; 11+ messages in thread
From: Jeff Law @ 2018-09-06 17:05 UTC (permalink / raw)
  To: John David Anglin, GCC Patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-06 17:05 ` Jeff Law
@ 2018-09-10 12:35   ` Andreas Schwab
  2018-09-10 14:10     ` John David Anglin
  2018-09-20  1:51   ` [PATCH] hppa: Remove TARGET_SCHED_ADJUST_PRIORITY hook John David Anglin
  1 sibling, 1 reply; 11+ messages in thread
From: Andreas Schwab @ 2018-09-10 12:35 UTC (permalink / raw)
  To: Jeff Law; +Cc: John David Anglin, GCC Patches

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."

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-10 12:35   ` Andreas Schwab
@ 2018-09-10 14:10     ` John David Anglin
  2018-09-17  9:13       ` Andreas Schwab
  0 siblings, 1 reply; 11+ messages in thread
From: John David Anglin @ 2018-09-10 14:10 UTC (permalink / raw)
  To: Andreas Schwab, Jeff Law; +Cc: GCC Patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-10 14:10     ` John David Anglin
@ 2018-09-17  9:13       ` Andreas Schwab
  2018-09-17 14:07         ` John David Anglin
  2018-09-17 19:34         ` Jeff Law
  0 siblings, 2 replies; 11+ messages in thread
From: Andreas Schwab @ 2018-09-17  9:13 UTC (permalink / raw)
  To: John David Anglin; +Cc: Jeff Law, GCC Patches

	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."

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-17  9:13       ` Andreas Schwab
@ 2018-09-17 14:07         ` John David Anglin
  2018-09-17 14:25           ` Andreas Schwab
  2018-09-17 19:34         ` Jeff Law
  1 sibling, 1 reply; 11+ messages in thread
From: John David Anglin @ 2018-09-17 14:07 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: Jeff Law, GCC Patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-17 14:07         ` John David Anglin
@ 2018-09-17 14:25           ` Andreas Schwab
  0 siblings, 0 replies; 11+ messages in thread
From: Andreas Schwab @ 2018-09-17 14:25 UTC (permalink / raw)
  To: John David Anglin; +Cc: Jeff Law, GCC Patches

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."

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-17  9:13       ` Andreas Schwab
  2018-09-17 14:07         ` John David Anglin
@ 2018-09-17 19:34         ` Jeff Law
  2018-09-18  8:00           ` Andreas Schwab
  1 sibling, 1 reply; 11+ messages in thread
From: Jeff Law @ 2018-09-17 19:34 UTC (permalink / raw)
  To: Andreas Schwab, John David Anglin; +Cc: GCC Patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-17 19:34         ` Jeff Law
@ 2018-09-18  8:00           ` Andreas Schwab
  2018-09-18 13:51             ` Jeff Law
  0 siblings, 1 reply; 11+ messages in thread
From: Andreas Schwab @ 2018-09-18  8:00 UTC (permalink / raw)
  To: Jeff Law; +Cc: John David Anglin, GCC Patches

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."

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority
  2018-09-18  8:00           ` Andreas Schwab
@ 2018-09-18 13:51             ` Jeff Law
  0 siblings, 0 replies; 11+ messages in thread
From: Jeff Law @ 2018-09-18 13:51 UTC (permalink / raw)
  To: Andreas Schwab; +Cc: John David Anglin, GCC Patches

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH] hppa: Remove TARGET_SCHED_ADJUST_PRIORITY hook
  2018-09-06 17:05 ` Jeff Law
  2018-09-10 12:35   ` Andreas Schwab
@ 2018-09-20  1:51   ` John David Anglin
  1 sibling, 0 replies; 11+ messages in thread
From: John David Anglin @ 2018-09-20  1:51 UTC (permalink / raw)
  To: Jeff Law, GCC Patches

[-- Attachment #1: Type: text/plain, Size: 354 bytes --]

On 2018-09-06 1:05 PM, Jeff Law wrote:
> 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.
The attached patch removes the TARGET_SCHED_ADJUST_PRIORITY hook.

Tested on hppa-unknown-linux-gnu.  Committed to trunk.

Dave

-- 
John David Anglin  dave.anglin@bell.net


[-- Attachment #2: pa_adjust_priority.d --]
[-- Type: text/plain, Size: 2146 bytes --]

2018-09-19  John David Anglin  <danglin@gcc.gnu.org>

	* config/pa/pa.c (pa_adjust_priority): Delete.
	(TARGET_SCHED_ADJUST_PRIORITY): Delete define.

Index: config/pa/pa.c
===================================================================
--- config/pa/pa.c	(revision 264168)
+++ config/pa/pa.c	(working copy)
@@ -122,7 +122,6 @@
 static void update_total_code_bytes (unsigned int);
 static void pa_output_function_epilogue (FILE *);
 static int pa_adjust_cost (rtx_insn *, int, rtx_insn *, int, unsigned int);
-static int pa_adjust_priority (rtx_insn *, int);
 static int pa_issue_rate (void);
 static int pa_reloc_rw_mask (void);
 static void pa_som_asm_init_sections (void) ATTRIBUTE_UNUSED;
@@ -280,8 +279,6 @@
 
 #undef TARGET_SCHED_ADJUST_COST
 #define TARGET_SCHED_ADJUST_COST pa_adjust_cost
-#undef TARGET_SCHED_ADJUST_PRIORITY
-#define TARGET_SCHED_ADJUST_PRIORITY pa_adjust_priority
 #undef TARGET_SCHED_ISSUE_RATE
 #define TARGET_SCHED_ISSUE_RATE pa_issue_rate
 
@@ -4995,37 +4992,6 @@
     }
 }
 
-/* Adjust scheduling priorities.  We use this to try and keep addil
-   and the next use of %r1 close together.  */
-static int
-pa_adjust_priority (rtx_insn *insn, int priority)
-{
-  rtx set = single_set (insn);
-  rtx src, dest;
-  if (set)
-    {
-      src = SET_SRC (set);
-      dest = SET_DEST (set);
-      if (GET_CODE (src) == LO_SUM
-	  && symbolic_operand (XEXP (src, 1), VOIDmode)
-	  && ! read_only_operand (XEXP (src, 1), VOIDmode))
-	priority >>= 3;
-
-      else if (GET_CODE (src) == MEM
-	       && GET_CODE (XEXP (src, 0)) == LO_SUM
-	       && symbolic_operand (XEXP (XEXP (src, 0), 1), VOIDmode)
-	       && ! read_only_operand (XEXP (XEXP (src, 0), 1), VOIDmode))
-	priority >>= 1;
-
-      else if (GET_CODE (dest) == MEM
-	       && GET_CODE (XEXP (dest, 0)) == LO_SUM
-	       && symbolic_operand (XEXP (XEXP (dest, 0), 1), VOIDmode)
-	       && ! read_only_operand (XEXP (XEXP (dest, 0), 1), VOIDmode))
-	priority >>= 3;
-    }
-  return priority;
-}
-
 /* The 700 can only issue a single insn at a time.
    The 7XXX processors can issue two insns at a time.
    The 8000 can issue 4 insns at a time.  */

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2018-09-20  1:44 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-03 14:32 [PATCH]: Allow TARGET_SCHED_ADJUST_PRIORITY hook to reduce priority John David Anglin
2018-09-06 17:05 ` Jeff Law
2018-09-10 12:35   ` Andreas Schwab
2018-09-10 14:10     ` John David Anglin
2018-09-17  9:13       ` Andreas Schwab
2018-09-17 14:07         ` John David Anglin
2018-09-17 14:25           ` Andreas Schwab
2018-09-17 19:34         ` Jeff Law
2018-09-18  8:00           ` Andreas Schwab
2018-09-18 13:51             ` Jeff Law
2018-09-20  1:51   ` [PATCH] hppa: Remove TARGET_SCHED_ADJUST_PRIORITY hook John David Anglin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).