[PATCH][PR rtl-optimization/81308] Conditionally cleanup the CFG after insn splitting

[PATCH][PR rtl-optimization/81308] Conditionally cleanup the CFG after insn splitting

[Jeff Law]

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


This patch fixes the original problem reported in 81308.  Namely that
g++.dg/pr62079.C will trigger a checking failure on 32bit x86.

As Jakub noted in the BZ the problem is we had an insn with an EH region
note.  That insn gets split and the split insns do not have an EH region
note (nor do they need one AFAICT).

With the EH region note gone, the actual EH region becomes unreachable
and we get a verification error in the dominance code.

My solution is relatively simple.  During splitting we track if the
current insn has an EH region note.  If it does and we end splitting the
insn or deleting it as a nop-move, then we note that we're going to need
a cfg cleanup.

After splitting all insns, we conditionally cleanup the CFG.

This should keep the overhead relatively low -- primarily it's the cost
to look for the EH region note on each insn as I don't expect the cfg
cleanup is often needed.

If we could prove to ourselves that the situation only occurs with
-fnon-call-exceptions, then we could further reduce the overhead by
exploiting that invariant as well.  I haven't really thought too much
about this.

No new testcase as the existing pr62079.C will trigger on x86.

Bootstrapped and regression tested on x86_64.  Verified pr62079.C now
passes by hand in 32 bit mode.

OK for the trunk?

Jeff

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 2097 bytes --]

	PR rtl-optimization/81308
	* recog.c (split_all_insns): Conditionally cleanup the CFG after
	splitting insns.

diff --git a/gcc/recog.c b/gcc/recog.c
index d6aa903..cc28b71 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -2931,6 +2931,7 @@ void
 split_all_insns (void)
 {
   bool changed;
+  bool need_cfg_cleanup = false;
   basic_block bb;
 
   auto_sbitmap blocks (last_basic_block_for_fn (cfun));
@@ -2949,6 +2950,18 @@ split_all_insns (void)
 	     CODE_LABELS and short-out basic blocks.  */
 	  next = NEXT_INSN (insn);
 	  finish = (insn == BB_END (bb));
+
+	  /* If INSN has a REG_EH_REGION note and we split INSN, the
+	     resulting split may not have/need REG_EH_REGION notes.
+
+	     If that happens and INSN was the last reference to the
+	     given EH region, then the EH region will become unreachable.
+	     We can not leave the unreachable blocks in the CFG as that
+	     will trigger a checking failure.
+
+	     So track if INSN has a REG_EH_REGION note.  If so and we
+	     split INSN, then trigger a CFG cleanup.  */
+	  rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
 	  if (INSN_P (insn))
 	    {
 	      rtx set = single_set (insn);
@@ -2965,6 +2978,8 @@ split_all_insns (void)
 		     nops then anyways.  */
 		  if (reload_completed)
 		      delete_insn_and_edges (insn);
+		  if (note)
+		    need_cfg_cleanup = true;
 		}
 	      else
 		{
@@ -2972,6 +2987,8 @@ split_all_insns (void)
 		    {
 		      bitmap_set_bit (blocks, bb->index);
 		      changed = true;
+		      if (note)
+			need_cfg_cleanup = true;
 		    }
 		}
 	    }
@@ -2980,7 +2997,16 @@ split_all_insns (void)
 
   default_rtl_profile ();
   if (changed)
-    find_many_sub_basic_blocks (blocks);
+    {
+      find_many_sub_basic_blocks (blocks);
+
+      /* Splitting could drop an REG_EH_REGION if it potentially
+	 trapped in its original form, but does not in its split
+	 form.  Consider a FLOAT_TRUNCATE which splits into a memory
+	 store/load pair and -fnon-call-exceptions.  */
+      if (need_cfg_cleanup)
+	cleanup_cfg (0);
+    }
 
   checking_verify_flow_info ();
 }

Re: [PATCH][PR rtl-optimization/81308] Conditionally cleanup the CFG after insn splitting

[Richard Biener]

On Mon, Jan 8, 2018 at 5:22 AM, Jeff Law <law@redhat.com> wrote:
>
> This patch fixes the original problem reported in 81308.  Namely that
> g++.dg/pr62079.C will trigger a checking failure on 32bit x86.
>
> As Jakub noted in the BZ the problem is we had an insn with an EH region
> note.  That insn gets split and the split insns do not have an EH region
> note (nor do they need one AFAICT).
>
> With the EH region note gone, the actual EH region becomes unreachable
> and we get a verification error in the dominance code.
>
> My solution is relatively simple.  During splitting we track if the
> current insn has an EH region note.  If it does and we end splitting the
> insn or deleting it as a nop-move, then we note that we're going to need
> a cfg cleanup.
>
> After splitting all insns, we conditionally cleanup the CFG.
>
> This should keep the overhead relatively low -- primarily it's the cost
> to look for the EH region note on each insn as I don't expect the cfg
> cleanup is often needed.
>
> If we could prove to ourselves that the situation only occurs with
> -fnon-call-exceptions, then we could further reduce the overhead by
> exploiting that invariant as well.  I haven't really thought too much
> about this.
>
> No new testcase as the existing pr62079.C will trigger on x86.
>
> Bootstrapped and regression tested on x86_64.  Verified pr62079.C now
> passes by hand in 32 bit mode.
>
> OK for the trunk?

Ok.

Richard.

> Jeff
>
>         PR rtl-optimization/81308
>         * recog.c (split_all_insns): Conditionally cleanup the CFG after
>         splitting insns.
>
> diff --git a/gcc/recog.c b/gcc/recog.c
> index d6aa903..cc28b71 100644
> --- a/gcc/recog.c
> +++ b/gcc/recog.c
> @@ -2931,6 +2931,7 @@ void
>  split_all_insns (void)
>  {
>    bool changed;
> +  bool need_cfg_cleanup = false;
>    basic_block bb;
>
>    auto_sbitmap blocks (last_basic_block_for_fn (cfun));
> @@ -2949,6 +2950,18 @@ split_all_insns (void)
>              CODE_LABELS and short-out basic blocks.  */
>           next = NEXT_INSN (insn);
>           finish = (insn == BB_END (bb));
> +
> +         /* If INSN has a REG_EH_REGION note and we split INSN, the
> +            resulting split may not have/need REG_EH_REGION notes.
> +
> +            If that happens and INSN was the last reference to the
> +            given EH region, then the EH region will become unreachable.
> +            We can not leave the unreachable blocks in the CFG as that
> +            will trigger a checking failure.
> +
> +            So track if INSN has a REG_EH_REGION note.  If so and we
> +            split INSN, then trigger a CFG cleanup.  */
> +         rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX);
>           if (INSN_P (insn))
>             {
>               rtx set = single_set (insn);
> @@ -2965,6 +2978,8 @@ split_all_insns (void)
>                      nops then anyways.  */
>                   if (reload_completed)
>                       delete_insn_and_edges (insn);
> +                 if (note)
> +                   need_cfg_cleanup = true;
>                 }
>               else
>                 {
> @@ -2972,6 +2987,8 @@ split_all_insns (void)
>                     {
>                       bitmap_set_bit (blocks, bb->index);
>                       changed = true;
> +                     if (note)
> +                       need_cfg_cleanup = true;
>                     }
>                 }
>             }
> @@ -2980,7 +2997,16 @@ split_all_insns (void)
>
>    default_rtl_profile ();
>    if (changed)
> -    find_many_sub_basic_blocks (blocks);
> +    {
> +      find_many_sub_basic_blocks (blocks);
> +
> +      /* Splitting could drop an REG_EH_REGION if it potentially
> +        trapped in its original form, but does not in its split
> +        form.  Consider a FLOAT_TRUNCATE which splits into a memory
> +        store/load pair and -fnon-call-exceptions.  */
> +      if (need_cfg_cleanup)
> +       cleanup_cfg (0);
> +    }
>
>    checking_verify_flow_info ();
>  }
>