RFA PR middle-end/48770

RFA PR middle-end/48770

[Jeff Law]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1


This version incorporates suggestions from Bernd.  Basically we have
reload1.c set reload_completed internally rather than deferring it into
ira.c.  That allows the call to reload() to return whether or not a DCE
pass is desirable at the end of reload.

That in turn allows us to avoid the DF clumsiness of the previous version.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.

Bernd is still seeing some differences on mips64-linux; I've been unable
to reproduce those.  Bernd, if you can send me the dump files privately,
I'm more than happy to take a look at any remaining codegen differences
this patch is triggering.

Thanks,
Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN938LAAoJEBRtltQi2kC7JxIH/jexv1Wx3RZkba8fgBMbrYYg
QLPv273smckcvITNaOdMKSRRbq/8x+hiGI4VClYX3z1tGrlIaDf+n0S/mOGmMDc3
yjxeXRBf0F8QPmkt+QG+Ck6TH3+ya2OOWmP6/RNCBQdaf7ViVuBI+IlGzhEia1OH
YL+3yDTfLpAgJ9BYTpaIB8o9m/cAAx0Rfnwgx9gcQzFGSPgEep1tg+gnxoyMbvGX
IohygwiMkU27JLokeanowL9d2H7L0kYMX1S0biDOdlm1wLI9n3JfLO9PPF0SLv8A
EESCaRmeJRH93wlNLb5qpacESgQOc6B6++zCjf1W22/GVcZIe9WKaOuxKtsoU/I=
=ZEQd
-----END PGP SIGNATURE-----

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 6554 bytes --]

	PR middle-end/48770
	* reload.h (reload): Change to return a bool.
	* ira.c (ira): If requested by reload, run a fast DCE pass after
	reload has completed.  Fix comment typo.
	* reload1.c (need_dce): New file scoped static.
	(reload): Set reload_completed here.  Return whether or not a DCE
	pass after reload is needed.
	(delete_dead_insn): Set need_dce as needed.

	PR middle-end/48770
	* gcc.dg/pr48770.c: New test.

Index: reload.h
===================================================================
*** reload.h	(revision 174696)
--- reload.h	(working copy)
*************** extern void reload_cse_regs (rtx);
*** 420,426 ****
  extern void init_reload (void);
  
  /* The reload pass itself.  */
! extern int reload (rtx, int);
  
  /* Mark the slots in regs_ever_live for the hard regs
     used by pseudo-reg number REGNO.  */
--- 420,426 ----
  extern void init_reload (void);
  
  /* The reload pass itself.  */
! extern bool reload (rtx, int);
  
  /* Mark the slots in regs_ever_live for the hard regs
     used by pseudo-reg number REGNO.  */
Index: testsuite/gcc.dg/pr48770.c
===================================================================
*** testsuite/gcc.dg/pr48770.c	(revision 0)
--- testsuite/gcc.dg/pr48770.c	(revision 0)
***************
*** 0 ****
--- 1,21 ----
+ /* { dg-do run } */
+ /* { dg-options "-O -fprofile-arcs -fPIC -fno-dce -fno-forward-propagate" } */
+ 
+ int test_goto2 (int f)
+ {
+   int i;
+   for (i = 0; ({_Bool a = i < 10;a;}); i++)
+   {
+     if (i == f)
+       goto lab2;
+   }
+   return 4;
+ lab2:
+   return 8;
+ }
+ 
+ int main ()
+ {
+   test_goto2 (30);
+   return 0;
+ }
Index: ira.c
===================================================================
*** ira.c	(revision 174759)
--- ira.c	(working copy)
*************** along with GCC; see the file COPYING3.  
*** 383,388 ****
--- 383,389 ----
  #include "integrate.h"
  #include "ggc.h"
  #include "ira-int.h"
+ #include "dce.h"
  
  
  struct target_ira default_target_ira;
*************** ira (FILE *f)
*** 3526,3531 ****
--- 3527,3533 ----
    int rebuild_p;
    int saved_flag_ira_share_spill_slots;
    basic_block bb;
+   bool need_dce;
  
    timevar_push (TV_IRA);
  
*************** ira (FILE *f)
*** 3717,3723 ****
    df_set_flags (DF_NO_INSN_RESCAN);
    build_insn_chain ();
  
!   reload_completed = !reload (get_insns (), ira_conflicts_p);
  
    timevar_pop (TV_RELOAD);
  
--- 3719,3725 ----
    df_set_flags (DF_NO_INSN_RESCAN);
    build_insn_chain ();
  
!   need_dce = reload (get_insns (), ira_conflicts_p);
  
    timevar_pop (TV_RELOAD);
  
*************** ira (FILE *f)
*** 3760,3766 ****
  #endif
  
    /* The code after the reload has changed so much that at this point
!      we might as well just rescan everything.  Not that
       df_rescan_all_insns is not going to help here because it does not
       touch the artificial uses and defs.  */
    df_finish_pass (true);
--- 3762,3768 ----
  #endif
  
    /* The code after the reload has changed so much that at this point
!      we might as well just rescan everything.  Note that
       df_rescan_all_insns is not going to help here because it does not
       touch the artificial uses and defs.  */
    df_finish_pass (true);
*************** ira (FILE *f)
*** 3772,3777 ****
--- 3774,3782 ----
    if (optimize)
      df_analyze ();
  
+   if (need_dce && optimize)
+     run_fast_dce ();
+ 
    timevar_pop (TV_IRA);
  }
  
Index: reload1.c
===================================================================
*** reload1.c	(revision 174759)
--- reload1.c	(working copy)
*************** static char *reload_insn_firstobj;
*** 250,255 ****
--- 250,259 ----
     examine.  */
  struct insn_chain *reload_insn_chain;
  
+ /* TRUE if we potentially left dead insns in the insn stream and want to
+    run DCE immediately after reload, FALSE otherwise.  */
+ static bool need_dce;
+ 
  /* List of all insns needing reloads.  */
  static struct insn_chain *insns_need_reload;
  \f
*************** static int *temp_pseudo_reg_arr;
*** 695,704 ****
     If GLOBAL is zero, we do not have enough information to do that,
     so any pseudo reg that is spilled must go to the stack.
  
!    Return value is nonzero if reload failed
!    and we must not do any more for this function.  */
  
! int
  reload (rtx first, int global)
  {
    int i, n;
--- 699,709 ----
     If GLOBAL is zero, we do not have enough information to do that,
     so any pseudo reg that is spilled must go to the stack.
  
!    Return value is TRUE if reload likely left dead insns in the
!    stream and a DCE pass should be run to elimiante them.  Else the
!    return value is FALSE.  */
  
! bool
  reload (rtx first, int global)
  {
    int i, n;
*************** reload (rtx first, int global)
*** 1329,1335 ****
  
    gcc_assert (bitmap_empty_p (&spilled_pseudos));
  
!   return failure;
  }
  
  /* Yet another special case.  Unfortunately, reg-stack forces people to
--- 1334,1342 ----
  
    gcc_assert (bitmap_empty_p (&spilled_pseudos));
  
!   reload_completed = !failure;
! 
!   return need_dce;
  }
  
  /* Yet another special case.  Unfortunately, reg-stack forces people to
*************** delete_dead_insn (rtx insn)
*** 2123,2136 ****
    rtx prev = prev_active_insn (insn);
    rtx prev_dest;
  
!   /* If the previous insn sets a register that dies in our insn, delete it
!      too.  */
    if (prev && GET_CODE (PATTERN (prev)) == SET
        && (prev_dest = SET_DEST (PATTERN (prev)), REG_P (prev_dest))
        && reg_mentioned_p (prev_dest, PATTERN (insn))
        && find_regno_note (insn, REG_DEAD, REGNO (prev_dest))
        && ! side_effects_p (SET_SRC (PATTERN (prev))))
!     delete_dead_insn (prev);
  
    SET_INSN_DELETED (insn);
  }
--- 2130,2148 ----
    rtx prev = prev_active_insn (insn);
    rtx prev_dest;
  
!   /* If the previous insn sets a register that dies in our insn make
!      a note that we want to run DCE immediately after reload.
! 
!      We used to delete the previous insn & recurse, but that's wrong for
!      block local equivalences.  Instead of trying to figure out the exact
!      circumstances where we can delete the potentially dead insns, just
!      let DCE do the job.  */
    if (prev && GET_CODE (PATTERN (prev)) == SET
        && (prev_dest = SET_DEST (PATTERN (prev)), REG_P (prev_dest))
        && reg_mentioned_p (prev_dest, PATTERN (insn))
        && find_regno_note (insn, REG_DEAD, REGNO (prev_dest))
        && ! side_effects_p (SET_SRC (PATTERN (prev))))
!     need_dce = 1;
  
    SET_INSN_DELETED (insn);
  }

Re: RFA PR middle-end/48770

[Bernd Schmidt]

On 06/14/2011 05:32 PM, Jeff Law wrote:
> 
> This version incorporates suggestions from Bernd.  Basically we have
> reload1.c set reload_completed internally rather than deferring it into
> ira.c.  That allows the call to reload() to return whether or not a DCE
> pass is desirable at the end of reload.
> 
> That in turn allows us to avoid the DF clumsiness of the previous version.
> 
> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.

This is OK, although I'm slightly confused - wasn't your original
problem caused by the other delete_dead_insn call?


Bernd

Re: RFA PR middle-end/48770

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 06/22/11 08:25, Bernd Schmidt wrote:
> On 06/14/2011 05:32 PM, Jeff Law wrote:
>>
>> This version incorporates suggestions from Bernd.  Basically we have
>> reload1.c set reload_completed internally rather than deferring it into
>> ira.c.  That allows the call to reload() to return whether or not a DCE
>> pass is desirable at the end of reload.
>>
>> That in turn allows us to avoid the DF clumsiness of the previous version.
>>
>> Bootstrapped and regression tested on x86_64-unknown-linux-gnu.
> 
> This is OK, although I'm slightly confused - wasn't your original
> problem caused by the other delete_dead_insn call?
It was caused by the recursive nature of delete_dead_insn.  Removing the
equivalencing insn is OK, it's deleting the insns feeding the
equivalencing insn that causes problems.

We could have delete_dead_insn not remove the equivalencing insn, but
that's going to trigger a lot more calls to DCE after reload has
completed and thus a higher compile-time hit.

jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJOA6ZwAAoJEBRtltQi2kC7U50H/AlscEfPiXJr3m89IEMp/eO3
C7NNc6WRaUPc/6j7T36xlryJd9H01VBKtuVDNHMAutWfosVhqufEid6a94okLbFG
IWsd2Bd1UTiqt9l0ddklo3/9XYmQoKlYmoYi+5XrvjGi+RQAByThA1j2agehMri7
ENxaYuvsAiitdl7Uan5hkTCcM0u4oLU1xBTbDa37o0Z0XDwVl3m4t47cwIQxpvWD
HZD5VvvafgvLRsmtRdJ8lipujicNR1uV2eblF08ZBnf8gat4AQ7/0+SLKLrLN1bN
9RIB7YJHgm2yxTnZ0pms1+elkTMvqVKhy+Mvk8u7njsRGpixgavhhlFfbtKvVeo=
=K8kI
-----END PGP SIGNATURE-----

Re: RFA PR middle-end/48770

[Jeff Law]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 05/30/11 04:55, Bernd Schmidt wrote:
> On 05/27/2011 07:49 PM, Jeff Law wrote:
>> Updated based on some comments from Bernd; specifically the other use of
>> delete_dead_insn has been removed.
>>
>> WRT the assembly differences on MIPS Bernd referred to; what ultimately
>> caused this problem were two dead insns that had been previously
>> eliminated by reload were still in the insn stream and inhibited an
>> if-conversion which resulted in slightly different assembly code, but
>> shouldn't have had any significant impact on the performance or size of
>> the resulting code.  The dead insns were deleted by the post-reload DCE
>> pass (which obviously runs after post-reload if conversion).
>>
>> If we really wanted to get those insns out of the stream, we could flag
>> when reload deleted insns which might result in dead code remaining in
>> the stream, then conditionally run DCE immediately after reload.  I
>> didn't think this was worth doing right now, but if someone objects I
>> can certainly look into it.
> 
> Patch is OK (still no codegen changes on x86 as far as I can tell), but
> I'd appreciate the DCE thing as a followup.
I suspect it's going to be just as easy to go ahead and add the DCE
cleanup now rather than waiting.  It should be a relatively trivial
tweak to delete_dead_insn to set a flag which we'd check at the end of
reload.  It's #2 in the queue :-)


jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN5SmlAAoJEBRtltQi2kC7RfwH/RV9OsQIwMeKOHUOgOkpvQZx
DIjRjlGTld+ndDN7sjgt0bIVLBITdR2K1CcVyPMDCZ2qZB0yiuDbXDQOZJkdrq89
55BUKrK7h5KLI5Ad8z5cGsyeK5nY3TUXmuPsdl0Pk/Lbu5m+EfxxxuQw0EljBhY9
UOasXcQ8VAXut0k4banTNFrY07TCM3gHMN2qRKTGiabtq/gh17ez/DejTVt3+2Ru
1Qi9XUVpL1mLwWsrqhI4JIe/zhpJS4eJizvP4Dywp+0pdplSPJVV9tybFWiojdfP
zt/4IQD0YHosTdxIEPyLWZBuszyG0TDmsQYyt29cXYzQeZSge+T1CF9TfEcX1wY=
=tSMU
-----END PGP SIGNATURE-----

Re: RFA PR middle-end/48770

[Bernd Schmidt]

On 05/27/2011 07:49 PM, Jeff Law wrote:
> Updated based on some comments from Bernd; specifically the other use of
> delete_dead_insn has been removed.
> 
> WRT the assembly differences on MIPS Bernd referred to; what ultimately
> caused this problem were two dead insns that had been previously
> eliminated by reload were still in the insn stream and inhibited an
> if-conversion which resulted in slightly different assembly code, but
> shouldn't have had any significant impact on the performance or size of
> the resulting code.  The dead insns were deleted by the post-reload DCE
> pass (which obviously runs after post-reload if conversion).
> 
> If we really wanted to get those insns out of the stream, we could flag
> when reload deleted insns which might result in dead code remaining in
> the stream, then conditionally run DCE immediately after reload.  I
> didn't think this was worth doing right now, but if someone objects I
> can certainly look into it.

Patch is OK (still no codegen changes on x86 as far as I can tell), but
I'd appreciate the DCE thing as a followup.


Bernd

RFA PR middle-end/48770

[Jeff Law]

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

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Updated based on some comments from Bernd; specifically the other use of
delete_dead_insn has been removed.

WRT the assembly differences on MIPS Bernd referred to; what ultimately
caused this problem were two dead insns that had been previously
eliminated by reload were still in the insn stream and inhibited an
if-conversion which resulted in slightly different assembly code, but
shouldn't have had any significant impact on the performance or size of
the resulting code.  The dead insns were deleted by the post-reload DCE
pass (which obviously runs after post-reload if conversion).

If we really wanted to get those insns out of the stream, we could flag
when reload deleted insns which might result in dead code remaining in
the stream, then conditionally run DCE immediately after reload.  I
didn't think this was worth doing right now, but if someone objects I
can certainly look into it.

Bootstrapped and regression tested on x86_64-unknown-linux-gnu.

Jeff
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Fedora - http://enigmail.mozdev.org/

iQEcBAEBAgAGBQJN3+RCAAoJEBRtltQi2kC7qUMH/3Z8NB8bogEnVvS7qF6eubu0
UNWp2HzJ/QnvxgKQiJUBar/kOF3y20kl2oZpqIoPVea2TqmeZ/lfuS1+lzWWxC/t
HGvjBWfkA60DkcDxFmHn0TadoqL11eAWEzsRRf/HfEn6I3rnrO3vP0RR3X8Dtiqe
qeFf8HVY8ysQoaFTyaiMLLo7yePmvlVy+HTsg+s3F0Yvg18P2WoJjpM/EY2y/tUR
QmIcFi8q4Q92YsfBpjweY6NyqepKrATAZT+CcFdc7GFvfa5TZQj9gGhWqPHiHQ5w
3c8RmKbaGEWhPSjIgmcqtY/FpY2CoRDn2BMosr8N4F0FMN2PkQlU8E7hTE2V6eM=
=UeKy
-----END PGP SIGNATURE-----

[-- Attachment #2: patch --]
[-- Type: text/plain, Size: 5261 bytes --]

	PR middle-end/48770
	* ira.c (update_equiv_regs): Update comment.
	* reload1.c (reload): Don't call delete_dead_insn to delete
	insns which create equivalences.
	(eliminate_regs_in_insn): Likewise.
	(delete_dead_insn): Remove.

	PR middle-end/48770
	* gcc.dg/pr48770.c: New test.

Index: testsuite/gcc.dg/pr48770.c
===================================================================
*** testsuite/gcc.dg/pr48770.c	(revision 0)
--- testsuite/gcc.dg/pr48770.c	(revision 0)
***************
*** 0 ****
--- 1,21 ----
+ /* { dg-do run } */
+ /* { dg-options "-O -fprofile-arcs -fPIC -fno-dce -fno-forward-propagate" } */
+ 
+ int test_goto2 (int f)
+ {
+   int i;
+   for (i = 0; ({_Bool a = i < 10;a;}); i++)
+   {
+     if (i == f)
+       goto lab2;
+   }
+   return 4;
+ lab2:
+   return 8;
+ }
+ 
+ int main ()
+ {
+   test_goto2 (30);
+   return 0;
+ }
Index: ira.c
===================================================================
*** ira.c	(revision 174066)
--- ira.c	(working copy)
*************** update_equiv_regs (void)
*** 2842,2848 ****
  	     If we don't have a REG_EQUIV note, see if this insn is loading
  	     a register used only in one basic block from a MEM.  If so, and the
  	     MEM remains unchanged for the life of the register, add a REG_EQUIV
! 	     note.  */
  
  	  note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
  
--- 2842,2848 ----
  	     If we don't have a REG_EQUIV note, see if this insn is loading
  	     a register used only in one basic block from a MEM.  If so, and the
  	     MEM remains unchanged for the life of the register, add a REG_EQUIV
! 	     note, this creates a block local equivalence.  */
  
  	  note = find_reg_note (insn, REG_EQUIV, NULL_RTX);
  
Index: reload1.c
===================================================================
*** reload1.c	(revision 174066)
--- reload1.c	(working copy)
*************** static void delete_caller_save_insns (vo
*** 356,362 ****
  
  static void spill_failure (rtx, enum reg_class);
  static void count_spilled_pseudo (int, int, int);
- static void delete_dead_insn (rtx);
  static void alter_reg (int, int, bool);
  static void set_label_offsets (rtx, rtx, int);
  static void check_eliminable_occurrences (rtx);
--- 356,361 ----
*************** reload (rtx first, int global)
*** 1011,1021 ****
  	mark_elimination (ep->from, ep->to);
  
    /* If a pseudo has no hard reg, delete the insns that made the equivalence.
!      If that insn didn't set the register (i.e., it copied the register to
!      memory), just delete that insn instead of the equivalencing insn plus
!      anything now dead.  If we call delete_dead_insn on that insn, we may
!      delete the insn that actually sets the register if the register dies
!      there and that is incorrect.  */
  
    for (i = FIRST_PSEUDO_REGISTER; i < max_regno; i++)
      {
--- 1010,1024 ----
  	mark_elimination (ep->from, ep->to);
  
    /* If a pseudo has no hard reg, delete the insns that made the equivalence.
!      We used to recursively delete anything else now dead.  That is incorrect
!      if that insn didn't set the register (i.e., it copied the register to
!      memory); it is also incorrect for a block local equivalence as the memory
!      address may refer to another pseudo which still needs proper
!      initialization. 
! 
!      That code predated running dead code elimination after reload, so rather
!      that trying to identify all the dead code here (and get it wrong), just
!      delete the equivalencing insn and let DCE do its job.  */
  
    for (i = FIRST_PSEUDO_REGISTER; i < max_regno; i++)
      {
*************** reload (rtx first, int global)
*** 1034,1041 ****
  	      if (NOTE_P (equiv_insn)
  		  || can_throw_internal (equiv_insn))
  		;
- 	      else if (reg_set_p (regno_reg_rtx[i], PATTERN (equiv_insn)))
- 		delete_dead_insn (equiv_insn);
  	      else
  		SET_INSN_DELETED (equiv_insn);
  	    }
--- 1037,1042 ----
*************** spill_failure (rtx insn, enum reg_class 
*** 2114,2140 ****
      }
  }
  \f
- /* Delete an unneeded INSN and any previous insns who sole purpose is loading
-    data that is dead in INSN.  */
- 
- static void
- delete_dead_insn (rtx insn)
- {
-   rtx prev = prev_active_insn (insn);
-   rtx prev_dest;
- 
-   /* If the previous insn sets a register that dies in our insn, delete it
-      too.  */
-   if (prev && GET_CODE (PATTERN (prev)) == SET
-       && (prev_dest = SET_DEST (PATTERN (prev)), REG_P (prev_dest))
-       && reg_mentioned_p (prev_dest, PATTERN (insn))
-       && find_regno_note (insn, REG_DEAD, REGNO (prev_dest))
-       && ! side_effects_p (SET_SRC (PATTERN (prev))))
-     delete_dead_insn (prev);
- 
-   SET_INSN_DELETED (insn);
- }
- 
  /* Modify the home of pseudo-reg I.
     The new home is present in reg_renumber[I].
  
--- 2115,2120 ----
*************** eliminate_regs_in_insn (rtx insn, int re
*** 3319,3325 ****
  	       process it since it won't be used unless something changes.  */
  	    if (replace)
  	      {
! 		delete_dead_insn (insn);
  		return 1;
  	      }
  	    val = 1;
--- 3299,3305 ----
  	       process it since it won't be used unless something changes.  */
  	    if (replace)
  	      {
! 		SET_INSN_DELETED (insn);
  		return 1;
  	      }
  	    val = 1;