[PATCH, LRA]: Revert the revert of removal of usless move insns.

[PATCH, LRA]: Revert the revert of removal of usless move insns.

[Uros Bizjak]

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

Hello!

Before the recent patch to post-reload mode switching, vzeroupper
insertion depended on the existence of the return copy instructions
pair in functions that return a value. The first instruction in the
pair represents a move to a function return hard register, and the
second was a USE of the function return hard register. Sometimes a nop
move was generated (e.g. %eax->%eax) for the first instruction of the
return copy instructions pair and the patch [1] teached LRA  to remove
these useless instructions on the fly.

The removal caused optimize mode switching to trigger the assert,
since the first instruction of a return pair was not found. The
relevant part of the patch was later reverted. With the recent
optimize mode switching patch, this is no longer necessary for
vzeroupper insertion pass, so attached patch reverts the revert.

2018-11-21  Uros Bizjak  <ubizjak@gmail.com>

    Revert the revert:
    2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>

    Revert:
    2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>

    * lra-spills.c (lra_final_code_change): Remove useless move insns.

Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.

OK for mainline?

[1] https://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html

Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 1035 bytes --]

diff --git a/gcc/lra-spills.c b/gcc/lra-spills.c
index 33caf9f45649..008d7399687d 100644
--- a/gcc/lra-spills.c
+++ b/gcc/lra-spills.c
@@ -740,6 +740,7 @@ lra_final_code_change (void)
   int i, hard_regno;
   basic_block bb;
   rtx_insn *insn, *curr;
+  rtx set;
   int max_regno = max_reg_num ();
 
   for (i = FIRST_PSEUDO_REGISTER; i < max_regno; i++)
@@ -818,5 +819,19 @@ lra_final_code_change (void)
 	      }
 	  if (insn_change_p)
 	    lra_update_operator_dups (id);
+
+	  if ((set = single_set (insn)) != NULL
+	      && REG_P (SET_SRC (set)) && REG_P (SET_DEST (set))
+	      && REGNO (SET_SRC (set)) == REGNO (SET_DEST (set)))
+	    {
+	      /* Remove an useless move insn.  IRA can generate move
+		 insns involving pseudos.  It is better remove them
+		 earlier to speed up compiler a bit.  It is also
+		 better to do it here as they might not pass final RTL
+		 check in LRA, (e.g. insn moving a control register
+		 into itself).  */
+	      lra_invalidate_insn_data (insn);
+	      delete_insn (insn);
+	    }
 	}
 }

Re: [PATCH, LRA]: Revert the revert of removal of usless move insns.

[Vladimir Makarov]

On 11/21/2018 02:33 PM, Uros Bizjak wrote:
> Hello!
>
> Before the recent patch to post-reload mode switching, vzeroupper
> insertion depended on the existence of the return copy instructions
> pair in functions that return a value. The first instruction in the
> pair represents a move to a function return hard register, and the
> second was a USE of the function return hard register. Sometimes a nop
> move was generated (e.g. %eax->%eax) for the first instruction of the
> return copy instructions pair and the patch [1] teached LRA  to remove
> these useless instructions on the fly.
>
> The removal caused optimize mode switching to trigger the assert,
> since the first instruction of a return pair was not found. The
> relevant part of the patch was later reverted. With the recent
> optimize mode switching patch, this is no longer necessary for
> vzeroupper insertion pass, so attached patch reverts the revert.
>
> 2018-11-21  Uros Bizjak  <ubizjak@gmail.com>
>
>      Revert the revert:
>      2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>
>
>      Revert:
>      2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>
>
>      * lra-spills.c (lra_final_code_change): Remove useless move insns.
>
> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> OK for mainline?
Sure. Thank you, Uros.
> [1] https://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html
>
> Uros.

Re: [PATCH, LRA]: Revert the revert of removal of usless move insns.

[H.J. Lu]

On Wed, Nov 21, 2018 at 1:45 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>
>
>
> On 11/21/2018 02:33 PM, Uros Bizjak wrote:
> > Hello!
> >
> > Before the recent patch to post-reload mode switching, vzeroupper
> > insertion depended on the existence of the return copy instructions
> > pair in functions that return a value. The first instruction in the
> > pair represents a move to a function return hard register, and the
> > second was a USE of the function return hard register. Sometimes a nop
> > move was generated (e.g. %eax->%eax) for the first instruction of the
> > return copy instructions pair and the patch [1] teached LRA  to remove
> > these useless instructions on the fly.
> >
> > The removal caused optimize mode switching to trigger the assert,
> > since the first instruction of a return pair was not found. The
> > relevant part of the patch was later reverted. With the recent
> > optimize mode switching patch, this is no longer necessary for
> > vzeroupper insertion pass, so attached patch reverts the revert.
> >
> > 2018-11-21  Uros Bizjak  <ubizjak@gmail.com>
> >
> >      Revert the revert:
> >      2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>
> >
> >      Revert:
> >      2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>
> >
> >      * lra-spills.c (lra_final_code_change): Remove useless move insns.
> >
> > Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
> >
> > OK for mainline?
> Sure. Thank you, Uros.
> > [1] https://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html
> >
> > Uros.
>

This caused:

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=90178

-- 
H.J.

Re: [PATCH, LRA]: Revert the revert of removal of usless move insns.

[Vladimir Makarov]

On 11/21/18 2:33 PM, Uros Bizjak wrote:
> Hello!
>
> Before the recent patch to post-reload mode switching, vzeroupper
> insertion depended on the existence of the return copy instructions
> pair in functions that return a value. The first instruction in the
> pair represents a move to a function return hard register, and the
> second was a USE of the function return hard register. Sometimes a nop
> move was generated (e.g. %eax->%eax) for the first instruction of the
> return copy instructions pair and the patch [1] teached LRA  to remove
> these useless instructions on the fly.
>
> The removal caused optimize mode switching to trigger the assert,
> since the first instruction of a return pair was not found. The
> relevant part of the patch was later reverted. With the recent
> optimize mode switching patch, this is no longer necessary for
> vzeroupper insertion pass, so attached patch reverts the revert.
>
> 2018-11-21  Uros Bizjak  <ubizjak@gmail.com>
>
>      Revert the revert:
>      2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>
>
>      Revert:
>      2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>
>
>      * lra-spills.c (lra_final_code_change): Remove useless move insns.
>
> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>
> OK for mainline?
Sure, Uros. I support the patch.  But I think it would be wise to 
postpone its committing after releasing GCC-9.  Simply it is hard to 
predict the patch effect to other targets and I would avoid any risk at 
this stage.
> [1] https://gcc.gnu.org/ml/gcc-patches/2013-10/msg02208.html
>
> Uros.

Re: [PATCH, LRA]: Revert the revert of removal of usless move insns.

[Uros Bizjak]

On 4/20/19, Vladimir Makarov <vmakarov@redhat.com> wrote:
>
> On 11/21/18 2:33 PM, Uros Bizjak wrote:
>> Hello!
>>
>> Before the recent patch to post-reload mode switching, vzeroupper
>> insertion depended on the existence of the return copy instructions
>> pair in functions that return a value. The first instruction in the
>> pair represents a move to a function return hard register, and the
>> second was a USE of the function return hard register. Sometimes a nop
>> move was generated (e.g. %eax->%eax) for the first instruction of the
>> return copy instructions pair and the patch [1] teached LRA  to remove
>> these useless instructions on the fly.
>>
>> The removal caused optimize mode switching to trigger the assert,
>> since the first instruction of a return pair was not found. The
>> relevant part of the patch was later reverted. With the recent
>> optimize mode switching patch, this is no longer necessary for
>> vzeroupper insertion pass, so attached patch reverts the revert.
>>
>> 2018-11-21  Uros Bizjak  <ubizjak@gmail.com>
>>
>>      Revert the revert:
>>      2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>      Revert:
>>      2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>
>>
>>      * lra-spills.c (lra_final_code_change): Remove useless move insns.
>>
>> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>
>> OK for mainline?
> Sure, Uros. I support the patch.  But I think it would be wise to
> postpone its committing after releasing GCC-9.  Simply it is hard to
> predict the patch effect to other targets and I would avoid any risk at
> this stage.

Actually, the "revert of the revert" patch was already committed to
mainline some time ago.

To clear the possible misunderstanding, let me summarise the issue:

- the original patch that remove useless move insn caused a breakage
in vzeroupper pass.
- the original patch was reverted due to the above breakage
- the vzeroupper pass was later adjusted to tolerate removed useless
move instructions, and this cleared the way to revert the revert. Now
LRA removes useless move insns.

An orthogonal issue (PR90178) was discovered, showing that some passes
also depend on the presence of useless move insn.

The bisection stumbled on the "revert of the revert" patch that
(obviously) re-introduced the issue. I'm not in the position to decide
if useless move insn can be removed or if these later passes should be
fixed, I can only say that the vzeroupper pass is now agnostic to the
presence of useless move insns.

Uros.

Re: [PATCH, LRA]: Revert the revert of removal of usless move insns.

[Vladimir Makarov]

On 4/20/19 4:55 AM, Uros Bizjak wrote:
> On 4/20/19, Vladimir Makarov <vmakarov@redhat.com> wrote:
>> On 11/21/18 2:33 PM, Uros Bizjak wrote:
>>> Hello!
>>>
>>> Before the recent patch to post-reload mode switching, vzeroupper
>>> insertion depended on the existence of the return copy instructions
>>> pair in functions that return a value. The first instruction in the
>>> pair represents a move to a function return hard register, and the
>>> second was a USE of the function return hard register. Sometimes a nop
>>> move was generated (e.g. %eax->%eax) for the first instruction of the
>>> return copy instructions pair and the patch [1] teached LRA  to remove
>>> these useless instructions on the fly.
>>>
>>> The removal caused optimize mode switching to trigger the assert,
>>> since the first instruction of a return pair was not found. The
>>> relevant part of the patch was later reverted. With the recent
>>> optimize mode switching patch, this is no longer necessary for
>>> vzeroupper insertion pass, so attached patch reverts the revert.
>>>
>>> 2018-11-21  Uros Bizjak  <ubizjak@gmail.com>
>>>
>>>       Revert the revert:
>>>       2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>
>>>
>>>       Revert:
>>>       2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>
>>>
>>>       * lra-spills.c (lra_final_code_change): Remove useless move insns.
>>>
>>> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
>>>
>>> OK for mainline?
>> Sure, Uros. I support the patch.  But I think it would be wise to
>> postpone its committing after releasing GCC-9.  Simply it is hard to
>> predict the patch effect to other targets and I would avoid any risk at
>> this stage.
> Actually, the "revert of the revert" patch was already committed to
> mainline some time ago.

Sorry for confusion, Uros. I did not check the date of your original 
posting.  Insn removal was added to RA just to avoid wasting CPU cycles 
on such insn processing afterwards.  Such insns are removed anyway later 
in the pass pipeline.  The CPU time savings are tiny but the removal 
creates too many problems including new one PR90178.  I should have 
avoided to put this code in the first place.

I think we should remove this code forever. It is not convenient for me 
to do this now because I am traveling.  If somebody wants to remove the 
code, i am approving this in advance.


> To clear the possible misunderstanding, let me summarise the issue:
>
> - the original patch that remove useless move insn caused a breakage
> in vzeroupper pass.
> - the original patch was reverted due to the above breakage
> - the vzeroupper pass was later adjusted to tolerate removed useless
> move instructions, and this cleared the way to revert the revert. Now
> LRA removes useless move insns.
>
> An orthogonal issue (PR90178) was discovered, showing that some passes
> also depend on the presence of useless move insn.
>
> The bisection stumbled on the "revert of the revert" patch that
> (obviously) re-introduced the issue. I'm not in the position to decide
> if useless move insn can be removed or if these later passes should be
> fixed, I can only say that the vzeroupper pass is now agnostic to the
> presence of useless move insns.
>
> Uros.

[PATCH] LRA: Revert "Remove useless move insns"

[H.J. Lu]

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

On Sat, Apr 20, 2019 at 2:54 PM Vladimir Makarov <vmakarov@redhat.com> wrote:
>
>
> On 4/20/19 4:55 AM, Uros Bizjak wrote:
> > On 4/20/19, Vladimir Makarov <vmakarov@redhat.com> wrote:
> >> On 11/21/18 2:33 PM, Uros Bizjak wrote:
> >>> Hello!
> >>>
> >>> Before the recent patch to post-reload mode switching, vzeroupper
> >>> insertion depended on the existence of the return copy instructions
> >>> pair in functions that return a value. The first instruction in the
> >>> pair represents a move to a function return hard register, and the
> >>> second was a USE of the function return hard register. Sometimes a nop
> >>> move was generated (e.g. %eax->%eax) for the first instruction of the
> >>> return copy instructions pair and the patch [1] teached LRA  to remove
> >>> these useless instructions on the fly.
> >>>
> >>> The removal caused optimize mode switching to trigger the assert,
> >>> since the first instruction of a return pair was not found. The
> >>> relevant part of the patch was later reverted. With the recent
> >>> optimize mode switching patch, this is no longer necessary for
> >>> vzeroupper insertion pass, so attached patch reverts the revert.
> >>>
> >>> 2018-11-21  Uros Bizjak  <ubizjak@gmail.com>
> >>>
> >>>       Revert the revert:
> >>>       2013-10-26  Vladimir Makarov  <vmakarov@redhat.com>
> >>>
> >>>       Revert:
> >>>       2013-10-25  Vladimir Makarov  <vmakarov@redhat.com>
> >>>
> >>>       * lra-spills.c (lra_final_code_change): Remove useless move insns.
> >>>
> >>> Patch was bootstrapped and regression tested on x86_64-linux-gnu {,-m32}.
> >>>
> >>> OK for mainline?
> >> Sure, Uros. I support the patch.  But I think it would be wise to
> >> postpone its committing after releasing GCC-9.  Simply it is hard to
> >> predict the patch effect to other targets and I would avoid any risk at
> >> this stage.
> > Actually, the "revert of the revert" patch was already committed to
> > mainline some time ago.
>
> Sorry for confusion, Uros. I did not check the date of your original
> posting.  Insn removal was added to RA just to avoid wasting CPU cycles
> on such insn processing afterwards.  Such insns are removed anyway later
> in the pass pipeline.  The CPU time savings are tiny but the removal
> creates too many problems including new one PR90178.  I should have
> avoided to put this code in the first place.
>
> I think we should remove this code forever. It is not convenient for me
> to do this now because I am traveling.  If somebody wants to remove the
> code, i am approving this in advance.

I am checking in this patch.

Thanks.

>
> > To clear the possible misunderstanding, let me summarise the issue:
> >
> > - the original patch that remove useless move insn caused a breakage
> > in vzeroupper pass.
> > - the original patch was reverted due to the above breakage
> > - the vzeroupper pass was later adjusted to tolerate removed useless
> > move instructions, and this cleared the way to revert the revert. Now
> > LRA removes useless move insns.
> >
> > An orthogonal issue (PR90178) was discovered, showing that some passes
> > also depend on the presence of useless move insn.
> >
> > The bisection stumbled on the "revert of the revert" patch that
> > (obviously) re-introduced the issue. I'm not in the position to decide
> > if useless move insn can be removed or if these later passes should be
> > fixed, I can only say that the vzeroupper pass is now agnostic to the
> > presence of useless move insns.
> >
> > Uros.



-- 
H.J.

[-- Attachment #2: 0001-LRA-Revert-Remove-useless-move-insns.patch --]
[-- Type: application/x-patch, Size: 3865 bytes --]