[PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[James Cowgill]

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;

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[Jeff Law]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[James Cowgill]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[Jeff Law]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[James Cowgill]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[Jeff Law]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[Maciej W. Rozycki]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[James Cowgill]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[Maciej W. Rozycki]

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

Re: [PATCH] [PR rtl-optimization/65618] Fix MIPS ADA bootstrap failure

[James Cowgill]

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