* [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
@ 2016-12-19 15:44 James Cowgill
2016-12-19 21:47 ` Jeff Law
2017-01-11 16:50 ` Maciej W. Rozycki
0 siblings, 2 replies; 10+ messages in thread
From: James Cowgill @ 2016-12-19 15:44 UTC (permalink / raw)
To: gcc-patches
Hi,
This patch fixes PR 65618 where ADA cannot be bootstrapped natively on
mips due to a bootstrap comparison failure. The PR is currently in the
target component, but should be in the rtl-optimization component.
The underlying bug is in gcc/emit-rtl.c:try_split and is a result of
the fix for PR rtl-optimization/48826. In that PR, if a call_insn is
split into two instructions, the following NOTE_INSN_CALL_ARG_LOCATION
is moved so that it immediately follows the new call_insn. However,
after doing that the "after" variable was not updated and it could
still point to the old note instruction (the instruction after the
instruction to be split). The "after" variable is later used to obtain
the last instruction in the split and is then passed back to the
delayed branch scheduler influencing how delay slots are assigned. My
patch adjusts the code which handles the NOTE_INSN_CALL_ARG_LOCATION
note so that "after" is updated if necessary.
This bug causes the ADA bootstrap comparison failure in a-except.o
because the branch delay scheduling operates slightly differently for
that file if debug information is turned on.
Thanks,
James
gcc/Changelog:
2016-12-16 James Cowgill <James.Cowgill@imgtec.com>
PR rtl-optimization/65618
* emit-rtl.c (try_split): Update "after" when moving a
NOTE_INSN_CALL_ARG_LOCATION.
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 7de17454037..6be124ac038 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last)
next = NEXT_INSN (next))
if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
{
+ /* Advance after to the next instruction if it is about to
+ be removed. */
+ if (after == next)
+ after = NEXT_INSN (after);
+
remove_insn (next);
add_insn_after (next, insn, NULL);
break;
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2016-12-19 15:44 [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure James Cowgill
@ 2016-12-19 21:47 ` Jeff Law
2016-12-20 14:45 ` James Cowgill
2017-01-11 16:50 ` Maciej W. Rozycki
1 sibling, 1 reply; 10+ messages in thread
From: Jeff Law @ 2016-12-19 21:47 UTC (permalink / raw)
To: James Cowgill, gcc-patches
On 12/19/2016 08:44 AM, James Cowgill wrote:
> Hi,
>
> This patch fixes PR 65618 where ADA cannot be bootstrapped natively on
> mips due to a bootstrap comparison failure. The PR is currently in the
> target component, but should be in the rtl-optimization component.
>
> The underlying bug is in gcc/emit-rtl.c:try_split and is a result of
> the fix for PR rtl-optimization/48826. In that PR, if a call_insn is
> split into two instructions, the following NOTE_INSN_CALL_ARG_LOCATION
> is moved so that it immediately follows the new call_insn. However,
> after doing that the "after" variable was not updated and it could
> still point to the old note instruction (the instruction after the
> instruction to be split). The "after" variable is later used to obtain
> the last instruction in the split and is then passed back to the
> delayed branch scheduler influencing how delay slots are assigned. My
> patch adjusts the code which handles the NOTE_INSN_CALL_ARG_LOCATION
> note so that "after" is updated if necessary.
>
> This bug causes the ADA bootstrap comparison failure in a-except.o
> because the branch delay scheduling operates slightly differently for
> that file if debug information is turned on.
>
> Thanks,
> James
>
> gcc/Changelog:
>
> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com>
>
> PR rtl-optimization/65618
> * emit-rtl.c (try_split): Update "after" when moving a
> NOTE_INSN_CALL_ARG_LOCATION.
>
> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
> index 7de17454037..6be124ac038 100644
> --- a/gcc/emit-rtl.c
> +++ b/gcc/emit-rtl.c
> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last)
> next = NEXT_INSN (next))
> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
> {
> + /* Advance after to the next instruction if it is about to
> + be removed. */
> + if (after == next)
> + after = NEXT_INSN (after);
> +
> remove_insn (next);
> add_insn_after (next, insn, NULL);
> break;
>
So the thing I don't like when looking at this code is we set AFTER
immediately upon entry to try_split. But we don't use it until near the
very end of try_split. That's just asking for trouble.
Can we reasonably initialize AFTER just before it's used?
Jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2016-12-19 21:47 ` Jeff Law
@ 2016-12-20 14:45 ` James Cowgill
2017-01-01 22:27 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: James Cowgill @ 2016-12-20 14:45 UTC (permalink / raw)
To: gcc-patches
Hi,
On 19/12/16 21:43, Jeff Law wrote:
> On 12/19/2016 08:44 AM, James Cowgill wrote:
>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com>
>>
>> PR rtl-optimization/65618
>> * emit-rtl.c (try_split): Update "after" when moving a
>> NOTE_INSN_CALL_ARG_LOCATION.
>>
>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>> index 7de17454037..6be124ac038 100644
>> --- a/gcc/emit-rtl.c
>> +++ b/gcc/emit-rtl.c
>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last)
>> next = NEXT_INSN (next))
>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
>> {
>> + /* Advance after to the next instruction if it is about to
>> + be removed. */
>> + if (after == next)
>> + after = NEXT_INSN (after);
>> +
>> remove_insn (next);
>> add_insn_after (next, insn, NULL);
>> break;
>>
> So the thing I don't like when looking at this code is we set AFTER
> immediately upon entry to try_split. But we don't use it until near the
> very end of try_split. That's just asking for trouble.
>
> Can we reasonably initialize AFTER just before it's used?
I wasn't sure but looking closer I think that would be fine. This patch
also works and does what Richard Sandiford suggested in the PR.
2016-12-20 James Cowgill <James.Cowgill@imgtec.com>
PR rtl-optimization/65618
* emit-rtl.c (try_split): Move initialization of "before" and
"after" to just before the call to emit_insn_after_setloc.
diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 7de17454037..bdc984c65cf 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -3643,8 +3643,7 @@ mark_label_nuses (rtx x)
rtx_insn *
try_split (rtx pat, rtx_insn *trial, int last)
{
- rtx_insn *before = PREV_INSN (trial);
- rtx_insn *after = NEXT_INSN (trial);
+ rtx_insn *before, *after;
rtx note;
rtx_insn *seq, *tem;
int probability;
@@ -3818,6 +3817,9 @@ try_split (rtx pat, rtx_insn *trial, int last)
}
}
+ before = PREV_INSN (trial);
+ after = NEXT_INSN (trial);
+
tem = emit_insn_after_setloc (seq, trial, INSN_LOCATION (trial));
delete_insn (trial);
Thanks,
James
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2016-12-20 14:45 ` James Cowgill
@ 2017-01-01 22:27 ` Jeff Law
2017-01-03 11:04 ` James Cowgill
0 siblings, 1 reply; 10+ messages in thread
From: Jeff Law @ 2017-01-01 22:27 UTC (permalink / raw)
To: James Cowgill, gcc-patches
On 12/20/2016 07:38 AM, James Cowgill wrote:
> Hi,
>
> On 19/12/16 21:43, Jeff Law wrote:
>> On 12/19/2016 08:44 AM, James Cowgill wrote:
>>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com>
>>>
>>> PR rtl-optimization/65618
>>> * emit-rtl.c (try_split): Update "after" when moving a
>>> NOTE_INSN_CALL_ARG_LOCATION.
>>>
>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>>> index 7de17454037..6be124ac038 100644
>>> --- a/gcc/emit-rtl.c
>>> +++ b/gcc/emit-rtl.c
>>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last)
>>> next = NEXT_INSN (next))
>>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
>>> {
>>> + /* Advance after to the next instruction if it is about to
>>> + be removed. */
>>> + if (after == next)
>>> + after = NEXT_INSN (after);
>>> +
>>> remove_insn (next);
>>> add_insn_after (next, insn, NULL);
>>> break;
>>>
>> So the thing I don't like when looking at this code is we set AFTER
>> immediately upon entry to try_split. But we don't use it until near the
>> very end of try_split. That's just asking for trouble.
>>
>> Can we reasonably initialize AFTER just before it's used?
>
> I wasn't sure but looking closer I think that would be fine. This patch
> also works and does what Richard Sandiford suggested in the PR.
>
> 2016-12-20 James Cowgill <James.Cowgill@imgtec.com>
>
> PR rtl-optimization/65618
> * emit-rtl.c (try_split): Move initialization of "before" and
> "after" to just before the call to emit_insn_after_setloc.
OK.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2017-01-01 22:27 ` Jeff Law
@ 2017-01-03 11:04 ` James Cowgill
2017-01-03 18:41 ` Jeff Law
0 siblings, 1 reply; 10+ messages in thread
From: James Cowgill @ 2017-01-03 11:04 UTC (permalink / raw)
To: Jeff Law, gcc-patches
On 01/01/17 22:27, Jeff Law wrote:
> On 12/20/2016 07:38 AM, James Cowgill wrote:
>> Hi,
>>
>> On 19/12/16 21:43, Jeff Law wrote:
>>> On 12/19/2016 08:44 AM, James Cowgill wrote:
>>>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com>
>>>>
>>>> PR rtl-optimization/65618
>>>> * emit-rtl.c (try_split): Update "after" when moving a
>>>> NOTE_INSN_CALL_ARG_LOCATION.
>>>>
>>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>>>> index 7de17454037..6be124ac038 100644
>>>> --- a/gcc/emit-rtl.c
>>>> +++ b/gcc/emit-rtl.c
>>>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last)
>>>> next = NEXT_INSN (next))
>>>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
>>>> {
>>>> + /* Advance after to the next instruction if it is about to
>>>> + be removed. */
>>>> + if (after == next)
>>>> + after = NEXT_INSN (after);
>>>> +
>>>> remove_insn (next);
>>>> add_insn_after (next, insn, NULL);
>>>> break;
>>>>
>>> So the thing I don't like when looking at this code is we set AFTER
>>> immediately upon entry to try_split. But we don't use it until near the
>>> very end of try_split. That's just asking for trouble.
>>>
>>> Can we reasonably initialize AFTER just before it's used?
>>
>> I wasn't sure but looking closer I think that would be fine. This patch
>> also works and does what Richard Sandiford suggested in the PR.
>>
>> 2016-12-20 James Cowgill <James.Cowgill@imgtec.com>
>>
>> PR rtl-optimization/65618
>> * emit-rtl.c (try_split): Move initialization of "before" and
>> "after" to just before the call to emit_insn_after_setloc.
> OK.
Great. Can you commit this for me, since I don't have commit access?
Thanks,
James
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2017-01-03 11:04 ` James Cowgill
@ 2017-01-03 18:41 ` Jeff Law
0 siblings, 0 replies; 10+ messages in thread
From: Jeff Law @ 2017-01-03 18:41 UTC (permalink / raw)
To: James Cowgill, gcc-patches
On 01/03/2017 04:04 AM, James Cowgill wrote:
> On 01/01/17 22:27, Jeff Law wrote:
>> On 12/20/2016 07:38 AM, James Cowgill wrote:
>>> Hi,
>>>
>>> On 19/12/16 21:43, Jeff Law wrote:
>>>> On 12/19/2016 08:44 AM, James Cowgill wrote:
>>>>> 2016-12-16 James Cowgill <James.Cowgill@imgtec.com>
>>>>>
>>>>> PR rtl-optimization/65618
>>>>> * emit-rtl.c (try_split): Update "after" when moving a
>>>>> NOTE_INSN_CALL_ARG_LOCATION.
>>>>>
>>>>> diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
>>>>> index 7de17454037..6be124ac038 100644
>>>>> --- a/gcc/emit-rtl.c
>>>>> +++ b/gcc/emit-rtl.c
>>>>> @@ -3742,6 +3742,11 @@ try_split (rtx pat, rtx_insn *trial, int last)
>>>>> next = NEXT_INSN (next))
>>>>> if (NOTE_KIND (next) == NOTE_INSN_CALL_ARG_LOCATION)
>>>>> {
>>>>> + /* Advance after to the next instruction if it is about to
>>>>> + be removed. */
>>>>> + if (after == next)
>>>>> + after = NEXT_INSN (after);
>>>>> +
>>>>> remove_insn (next);
>>>>> add_insn_after (next, insn, NULL);
>>>>> break;
>>>>>
>>>> So the thing I don't like when looking at this code is we set AFTER
>>>> immediately upon entry to try_split. But we don't use it until near the
>>>> very end of try_split. That's just asking for trouble.
>>>>
>>>> Can we reasonably initialize AFTER just before it's used?
>>>
>>> I wasn't sure but looking closer I think that would be fine. This patch
>>> also works and does what Richard Sandiford suggested in the PR.
>>>
>>> 2016-12-20 James Cowgill <James.Cowgill@imgtec.com>
>>>
>>> PR rtl-optimization/65618
>>> * emit-rtl.c (try_split): Move initialization of "before" and
>>> "after" to just before the call to emit_insn_after_setloc.
>> OK.
>
> Great. Can you commit this for me, since I don't have commit access?
Done.
If you're going to be contributing regularly we should probably start
the process of getting you commit access.
jeff
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2016-12-19 15:44 [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure James Cowgill
2016-12-19 21:47 ` Jeff Law
@ 2017-01-11 16:50 ` Maciej W. Rozycki
2017-01-11 17:00 ` James Cowgill
1 sibling, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-01-11 16:50 UTC (permalink / raw)
To: James Cowgill; +Cc: gcc-patches
On Mon, 19 Dec 2016, James Cowgill wrote:
> This bug causes the ADA bootstrap comparison failure in a-except.o
> because the branch delay scheduling operates slightly differently for
> that file if debug information is turned on.
This looks like a bug to me -- actual code produced is supposed to be the
same regardless of the amount of debug information requested. This is
important for some debugging scenarios, such as when a binary originally
produced with no debug information crashes and a core file is obtained so
that the program can be rebuilt with debug information included and then
matched to the core file previously produced, which may not be easy to
recreate with the rebuilt binary.
From this consideration I gather you have a program source which can be
used as a test case to reproduce the issue, so can you please file a
problem report and include the source and a recipe to reproduce it? Is
this a GCC issue with generated assembly or a GAS issue with interpreting
it?
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2017-01-11 16:50 ` Maciej W. Rozycki
@ 2017-01-11 17:00 ` James Cowgill
2017-01-11 17:22 ` Maciej W. Rozycki
0 siblings, 1 reply; 10+ messages in thread
From: James Cowgill @ 2017-01-11 17:00 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gcc-patches
Hi,
On 11/01/17 16:49, Maciej W. Rozycki wrote:
> On Mon, 19 Dec 2016, James Cowgill wrote:
>> This bug causes the ADA bootstrap comparison failure in a-except.o
>> because the branch delay scheduling operates slightly differently for
>> that file if debug information is turned on.
>
> This looks like a bug to me -- actual code produced is supposed to be the
> same regardless of the amount of debug information requested. This is
> important for some debugging scenarios, such as when a binary originally
> produced with no debug information crashes and a core file is obtained so
> that the program can be rebuilt with debug information included and then
> matched to the core file previously produced, which may not be easy to
> recreate with the rebuilt binary.
>
> From this consideration I gather you have a program source which can be
> used as a test case to reproduce the issue, so can you please file a
> problem report and include the source and a recipe to reproduce it? Is
> this a GCC issue with generated assembly or a GAS issue with interpreting
> it?
Yes I had a testcase which I used to debug this, but it was pretty huge
(and written in ADA). When debugging I just diffed it with the debug
information toggled to see if the patch fixed it. I'll see if I can find
it anyway.
The issue was about the assembly GCC generated.
James
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2017-01-11 17:00 ` James Cowgill
@ 2017-01-11 17:22 ` Maciej W. Rozycki
2017-01-12 11:12 ` James Cowgill
0 siblings, 1 reply; 10+ messages in thread
From: Maciej W. Rozycki @ 2017-01-11 17:22 UTC (permalink / raw)
To: James Cowgill; +Cc: gcc-patches
On Wed, 11 Jan 2017, James Cowgill wrote:
> > From this consideration I gather you have a program source which can be
> > used as a test case to reproduce the issue, so can you please file a
> > problem report and include the source and a recipe to reproduce it? Is
> > this a GCC issue with generated assembly or a GAS issue with interpreting
> > it?
>
> Yes I had a testcase which I used to debug this, but it was pretty huge
> (and written in ADA). When debugging I just diffed it with the debug
> information toggled to see if the patch fixed it. I'll see if I can find
> it anyway.
A test case being huge is not an issue, we can always try to reduce it if
needed. What is key is reproducibility. Please make sure it's
self-contained, i.e. doesn't depend on system-installed components (for
C/C++ I'd ask for a preprocessed source, but I'm not sure offhand what the
requirements are for ADA -- it's been a while since I did anything about
ADA, and even then not a huge lot -- just a GCC 3.4 MIPS port).
> The issue was about the assembly GCC generated.
A GCC bug then.
Maciej
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure
2017-01-11 17:22 ` Maciej W. Rozycki
@ 2017-01-12 11:12 ` James Cowgill
0 siblings, 0 replies; 10+ messages in thread
From: James Cowgill @ 2017-01-12 11:12 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: gcc-patches
On 11/01/17 17:22, Maciej W. Rozycki wrote:
> On Wed, 11 Jan 2017, James Cowgill wrote:
>
>>> From this consideration I gather you have a program source which can be
>>> used as a test case to reproduce the issue, so can you please file a
>>> problem report and include the source and a recipe to reproduce it? Is
>>> this a GCC issue with generated assembly or a GAS issue with interpreting
>>> it?
>>
>> Yes I had a testcase which I used to debug this, but it was pretty huge
>> (and written in ADA). When debugging I just diffed it with the debug
>> information toggled to see if the patch fixed it. I'll see if I can find
>> it anyway.
>
> A test case being huge is not an issue, we can always try to reduce it if
> needed. What is key is reproducibility. Please make sure it's
> self-contained, i.e. doesn't depend on system-installed components (for
> C/C++ I'd ask for a preprocessed source, but I'm not sure offhand what the
> requirements are for ADA -- it's been a while since I did anything about
> ADA, and even then not a huge lot -- just a GCC 3.4 MIPS port).
Filed as PR/79071
James
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-01-12 11:12 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-12-19 15:44 [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure James Cowgill
2016-12-19 21:47 ` Jeff Law
2016-12-20 14:45 ` James Cowgill
2017-01-01 22:27 ` Jeff Law
2017-01-03 11:04 ` James Cowgill
2017-01-03 18:41 ` Jeff Law
2017-01-11 16:50 ` Maciej W. Rozycki
2017-01-11 17:00 ` James Cowgill
2017-01-11 17:22 ` Maciej W. Rozycki
2017-01-12 11:12 ` James Cowgill
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).