[v3] rtl-optimization/105231 - distribute_notes and REG_EH_REGION

  The following mitigates a problem in combine distribute_notes which
places an original REG_EH_REGION based on only may_trap_p which is
good to test whether a non-call insn can possibly throw but not if
actually it does or we care.  That's something we decided at RTL
expansion time where we possibly still know the insn evaluates
to a constant.

In fact, the REG_EH_REGION note with lp > 0 can only come from the
original i3 and an assert is added to that effect.  That means we only
need to retain the note on i3 or, if that cannot trap, drop it but we
should never move it to i2.

For REG_EH_REGION corresponding to must-not-throw regions or
nothrow marking try_combine gets new code ensuring we can merge
and distribute notes which means placing must-not-throw notes
on all result insns, and dropping nothrow notes or preserve
them on i3 for calls.

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

WDYT?

Thanks,
Richard.

2022-04-19  Richard Biener  <rguenther@suse.de>

	PR rtl-optimization/105231
	* combine.cc (distribute_notes): Assert that a REG_EH_REGION
	with landing pad > 0 is from i3 and only keep it there or drop
	it if the insn can not trap.  Throw away REG_EH_REGION with
	landing pad = 0 or INT_MIN if it does not originate from a
	call i3.  Distribute must-not-throw REG_EH_REGION to all
	resulting instructions.
	(try_combine): Ensure that we can merge REG_EH_REGION notes.

	* gcc.dg/torture/pr105231.c: New testcase.
---
 gcc/combine.cc                          | 106 ++++++++++++++++++++----
 gcc/testsuite/gcc.dg/torture/pr105231.c |  15 ++++
 2 files changed, 104 insertions(+), 17 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr105231.c
  

Hi!

This looks great :-)

On Wed, Apr 20, 2022 at 03:52:33PM +0200, Richard Biener wrote:
> The following mitigates a problem in combine distribute_notes which
> places an original REG_EH_REGION based on only may_trap_p which is
> good to test whether a non-call insn can possibly throw but not if
> actually it does or we care.  That's something we decided at RTL
> expansion time where we possibly still know the insn evaluates
> to a constant.
> 
> In fact, the REG_EH_REGION note with lp > 0 can only come from the
> original i3 and an assert is added to that effect.  That means we only
> need to retain the note on i3 or, if that cannot trap, drop it but we
> should never move it to i2.
> 
> For REG_EH_REGION corresponding to must-not-throw regions or
> nothrow marking try_combine gets new code ensuring we can merge
> and distribute notes which means placing must-not-throw notes
> on all result insns, and dropping nothrow notes or preserve
> them on i3 for calls.

> 	* combine.cc (distribute_notes): Assert that a REG_EH_REGION
> 	with landing pad > 0 is from i3 and only keep it there or drop
> 	it if the insn can not trap.  Throw away REG_EH_REGION with
> 	landing pad = 0 or INT_MIN if it does not originate from a
> 	call i3.  Distribute must-not-throw REG_EH_REGION to all
> 	resulting instructions.
> 	(try_combine): Ensure that we can merge REG_EH_REGION notes.

> --- a/gcc/combine.cc
> +++ b/gcc/combine.cc
> @@ -2951,6 +2951,45 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
>        return 0;
>      }
>  
> +  /* When i3 transfers to an EH handler we cannot combine if any of the
> +     sources are within a must-not-throw region.  Else we can throw away
> +     any nothrow, pick a random must-not-throw region or preserve the EH
> +     transfer on i3.  Since we want to preserve nothrow notes on calls
> +     we have to avoid combining from must-not-throw stmts there as well.
> +     This has to be kept in sync with distribute_note.  */
> +  if (rtx i3_eh = find_reg_note (i3, REG_EH_REGION, NULL_RTX))
> +    {
> +      int i3_lp_nr = INTVAL (XEXP (i3_eh, 0));
> +      if (i3_lp_nr > 0
> +	  || ((i3_lp_nr == 0 || i3_lp_nr == INT_MIN) && CALL_P (i3)))
> +	{
> +	  rtx eh;
> +	  int eh_lp;
> +	  if (((eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> +	       && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> +	       && eh_lp != INT_MIN)
> +	      || (i2
> +		  && (eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> +		  && eh_lp != INT_MIN)
> +	      || (i1
> +		  && (eh = find_reg_note (i1, REG_EH_REGION, NULL_RTX))
> +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> +		  && eh_lp != INT_MIN)
> +	      || (i0
> +		  && (eh = find_reg_note (i0, REG_EH_REGION, NULL_RTX))
> +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> +		  && eh_lp != INT_MIN))
> +	    {
> +	      if (dump_file && (dump_flags & TDF_DETAILS))
> +		fprintf (dump_file, "Can't combine insn in must-not-throw "
> +			 "EH region into i3 which can throws\n");
> +	      undo_all ();
> +	      return 0;
> +	    }
> +	}
> +    }

The assignments in the conditionals make this hard to read, and harder
to change, btw.  A utility function wouldn't hurt?  The problem of
course would be thinking of a good name for it :-)

>  	case REG_EH_REGION:
> -	  /* These notes must remain with the call or trapping instruction.  */
> -	  if (CALL_P (i3))
> -	    place = i3;
> -	  else if (i2 && CALL_P (i2))
> -	    place = i2;
> -	  else
> -	    {
> -	      gcc_assert (cfun->can_throw_non_call_exceptions);
> -	      if (may_trap_p (i3))
> -		place = i3;
> -	      else if (i2 && may_trap_p (i2))
> -		place = i2;
> -	      /* ??? Otherwise assume we've combined things such that we
> -		 can now prove that the instructions can't trap.  Drop the
> -		 note in this case.  */
> -	    }
> -	  break;
> +	  {
> +	    /* This handling needs to be kept in sync with the
> +	       prerequesite checking in try_combine.  */

(prerequisite)

> +	    int lp_nr = INTVAL (XEXP (note, 0));
> +	    /* A REG_EH_REGION note transfering control can only ever come
> +	       from i3 and it has to stay there.  */
> +	    if (lp_nr > 0)
> +	      {
> +		gcc_assert (from_insn == i3);
> +		if (CALL_P (i3))
> +		  place = i3;
> +		else
> +		  {
> +		    gcc_assert (cfun->can_throw_non_call_exceptions);
> +		    /* If i3 can still trap preserve the note, otherwise we've
> +		       combined things such that we can now prove that the
> +		       instructions can't trap.  Drop the note in this case.  */
> +		    if (may_trap_p (i3))
> +		      place = i3;
> +		  }
> +	      }

It isn't immediately clear to me that we cannot have moved a trapping
thing to i2?  Do we guarantee that somewhere?  Can we / should we assert
that here?  gcc_assert (!(i2 && may_trap_p (i2)))  or something like
that.

> +	    else if (lp_nr == 0 || lp_nr == INT_MIN)
> +	      {
> +		/* nothrow notes can be dropped but we preserve
> +		   those on calls.  */
> +		if (from_insn == i3 && CALL_P (i3))
> +		  place = i3;
> +	      }
> +	    else
> +	      {
> +		/* Distribute must-not-throw notes to all resulting
> +		   insns and just pick the first.  */
> +		if (!find_reg_note (i3, REG_EH_REGION, NULL_RTX)
> +		    && (CALL_P (i3)
> +			|| (cfun->can_throw_non_call_exceptions
> +			    && may_trap_p (i3))))
> +		  place = i3;
> +		if (i2
> +		    && !find_reg_note (i2, REG_EH_REGION, NULL_RTX)
> +		    && cfun->can_throw_non_call_exceptions
> +		    && may_trap_p (i2))
> +		  {
> +		    if (place)
> +		      place2 = i2;
> +		    else
> +		      place = i2;
> +		  }
> +	      }
> +	    break;
> +	  }


Okay for trunk with the spello fixed, and maybe one more assert added.
Thanks for all the work on this!


Segher
  

On Wed, 20 Apr 2022, Segher Boessenkool wrote:

> Hi!
> 
> This looks great :-)
> 
> On Wed, Apr 20, 2022 at 03:52:33PM +0200, Richard Biener wrote:
> > The following mitigates a problem in combine distribute_notes which
> > places an original REG_EH_REGION based on only may_trap_p which is
> > good to test whether a non-call insn can possibly throw but not if
> > actually it does or we care.  That's something we decided at RTL
> > expansion time where we possibly still know the insn evaluates
> > to a constant.
> > 
> > In fact, the REG_EH_REGION note with lp > 0 can only come from the
> > original i3 and an assert is added to that effect.  That means we only
> > need to retain the note on i3 or, if that cannot trap, drop it but we
> > should never move it to i2.
> > 
> > For REG_EH_REGION corresponding to must-not-throw regions or
> > nothrow marking try_combine gets new code ensuring we can merge
> > and distribute notes which means placing must-not-throw notes
> > on all result insns, and dropping nothrow notes or preserve
> > them on i3 for calls.
> 
> > 	* combine.cc (distribute_notes): Assert that a REG_EH_REGION
> > 	with landing pad > 0 is from i3 and only keep it there or drop
> > 	it if the insn can not trap.  Throw away REG_EH_REGION with
> > 	landing pad = 0 or INT_MIN if it does not originate from a
> > 	call i3.  Distribute must-not-throw REG_EH_REGION to all
> > 	resulting instructions.
> > 	(try_combine): Ensure that we can merge REG_EH_REGION notes.
> 
> > --- a/gcc/combine.cc
> > +++ b/gcc/combine.cc
> > @@ -2951,6 +2951,45 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
> >        return 0;
> >      }
> >  
> > +  /* When i3 transfers to an EH handler we cannot combine if any of the
> > +     sources are within a must-not-throw region.  Else we can throw away
> > +     any nothrow, pick a random must-not-throw region or preserve the EH
> > +     transfer on i3.  Since we want to preserve nothrow notes on calls
> > +     we have to avoid combining from must-not-throw stmts there as well.
> > +     This has to be kept in sync with distribute_note.  */
> > +  if (rtx i3_eh = find_reg_note (i3, REG_EH_REGION, NULL_RTX))
> > +    {
> > +      int i3_lp_nr = INTVAL (XEXP (i3_eh, 0));
> > +      if (i3_lp_nr > 0
> > +	  || ((i3_lp_nr == 0 || i3_lp_nr == INT_MIN) && CALL_P (i3)))
> > +	{
> > +	  rtx eh;
> > +	  int eh_lp;
> > +	  if (((eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > +	       && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > +	       && eh_lp != INT_MIN)
> > +	      || (i2
> > +		  && (eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > +		  && eh_lp != INT_MIN)
> > +	      || (i1
> > +		  && (eh = find_reg_note (i1, REG_EH_REGION, NULL_RTX))
> > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > +		  && eh_lp != INT_MIN)
> > +	      || (i0
> > +		  && (eh = find_reg_note (i0, REG_EH_REGION, NULL_RTX))
> > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > +		  && eh_lp != INT_MIN))
> > +	    {
> > +	      if (dump_file && (dump_flags & TDF_DETAILS))
> > +		fprintf (dump_file, "Can't combine insn in must-not-throw "
> > +			 "EH region into i3 which can throws\n");
> > +	      undo_all ();
> > +	      return 0;
> > +	    }
> > +	}
> > +    }
> 
> The assignments in the conditionals make this hard to read, and harder
> to change, btw.  A utility function wouldn't hurt?  The problem of
> course would be thinking of a good name for it :-)

I've added insn_must_not_throw_p to except.{cc,h}.

> >  	case REG_EH_REGION:
> > -	  /* These notes must remain with the call or trapping instruction.  */
> > -	  if (CALL_P (i3))
> > -	    place = i3;
> > -	  else if (i2 && CALL_P (i2))
> > -	    place = i2;
> > -	  else
> > -	    {
> > -	      gcc_assert (cfun->can_throw_non_call_exceptions);
> > -	      if (may_trap_p (i3))
> > -		place = i3;
> > -	      else if (i2 && may_trap_p (i2))
> > -		place = i2;
> > -	      /* ??? Otherwise assume we've combined things such that we
> > -		 can now prove that the instructions can't trap.  Drop the
> > -		 note in this case.  */
> > -	    }
> > -	  break;
> > +	  {
> > +	    /* This handling needs to be kept in sync with the
> > +	       prerequesite checking in try_combine.  */
> 
> (prerequisite)

Fixed.

> > +	    int lp_nr = INTVAL (XEXP (note, 0));
> > +	    /* A REG_EH_REGION note transfering control can only ever come
> > +	       from i3 and it has to stay there.  */
> > +	    if (lp_nr > 0)
> > +	      {
> > +		gcc_assert (from_insn == i3);
> > +		if (CALL_P (i3))
> > +		  place = i3;
> > +		else
> > +		  {
> > +		    gcc_assert (cfun->can_throw_non_call_exceptions);
> > +		    /* If i3 can still trap preserve the note, otherwise we've
> > +		       combined things such that we can now prove that the
> > +		       instructions can't trap.  Drop the note in this case.  */
> > +		    if (may_trap_p (i3))
> > +		      place = i3;
> > +		  }
> > +	      }
> 
> It isn't immediately clear to me that we cannot have moved a trapping
> thing to i2?  Do we guarantee that somewhere?  Can we / should we assert
> that here?  gcc_assert (!(i2 && may_trap_p (i2)))  or something like
> that.

IMHO asserts for catching things we don't handle are bad.  I suppose
I could add sth like

@@ -3724,7 +3712,12 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn 
*i1, rt
x_insn *i0,
              || !modified_between_p (*split, i2, i3))
          /* We can't overwrite I2DEST if its value is still used by
             NEWPAT.  */
-         && ! reg_referenced_p (i2dest, newpat))
+         && ! reg_referenced_p (i2dest, newpat)
+         /* We should not split a possibly trapping part of a throwing
+            insn.  */
+         && (!cfun->can_throw_non_call_exceptions
+             || !can_throw_internal (i3)
+             || !may_trap_p (*split)))
        {
          rtx newdest = i2dest;
          enum rtx_code split_code = GET_CODE (*split);

but then without a testcase it's hard to see if this is to the point.
It of course also assumes we're never going to split the call part
from i3 to i2.

It should be at least safe and with that we could add your assert,
sligthly altered to

                gcc_assert (!(cfun->can_throw_non_call_exceptions
                              && i2 && may_trap_p (i2)));

because we have to guard against spltting say a trapping memory
load out from a CALL_P, but only with non-call EH (the call
might not throw but a (pre-existing!) memory load could trap).

> > +	    else if (lp_nr == 0 || lp_nr == INT_MIN)
> > +	      {
> > +		/* nothrow notes can be dropped but we preserve
> > +		   those on calls.  */
> > +		if (from_insn == i3 && CALL_P (i3))
> > +		  place = i3;
> > +	      }
> > +	    else
> > +	      {
> > +		/* Distribute must-not-throw notes to all resulting
> > +		   insns and just pick the first.  */
> > +		if (!find_reg_note (i3, REG_EH_REGION, NULL_RTX)
> > +		    && (CALL_P (i3)
> > +			|| (cfun->can_throw_non_call_exceptions
> > +			    && may_trap_p (i3))))
> > +		  place = i3;
> > +		if (i2
> > +		    && !find_reg_note (i2, REG_EH_REGION, NULL_RTX)
> > +		    && cfun->can_throw_non_call_exceptions
> > +		    && may_trap_p (i2))
> > +		  {
> > +		    if (place)
> > +		      place2 = i2;
> > +		    else
> > +		      place = i2;
> > +		  }
> > +	      }
> > +	    break;
> > +	  }
> 
> 
> Okay for trunk with the spello fixed, and maybe one more assert added.
> Thanks for all the work on this!

I'm testing the following updated patch now.

OK?

Thanks,
Richard.

>From d7406b76a036e1aed2211751da27abcf1e5b0b86 Mon Sep 17 00:00:00 2001
From: Richard Biener <rguenther@suse.de>
Date: Thu, 14 Apr 2022 14:06:22 +0200
Subject: [PATCH] rtl-optimization/105231 - distribute_notes and REG_EH_REGION
To: gcc-patches@gcc.gnu.org

The following mitigates a problem in combine distribute_notes which
places an original REG_EH_REGION based on only may_trap_p which is
good to test whether a non-call insn can possibly throw but not if
actually it does or we care.  That's something we decided at RTL
expansion time where we possibly still know the insn evaluates
to a constant.

In fact, the REG_EH_REGION note with lp > 0 can only come from the
original i3 and an assert is added to that effect.  That means we only
need to retain the note on i3 or, if that cannot trap, drop it but we
should never move it to i2.

For REG_EH_REGION corresponding to must-not-throw regions or
nothrow marking try_combine gets new code ensuring we can merge
and distribute notes which means placing must-not-throw notes
on all result insns, and dropping nothrow notes or preserve
them on i3 for calls.

2022-04-19  Richard Biener  <rguenther@suse.de>

	PR rtl-optimization/105231
	* combine.cc (distribute_notes): Assert that a REG_EH_REGION
	with landing pad > 0 is from i3 and only keep it there or drop
	it if the insn can not trap.  Throw away REG_EH_REGION with
	landing pad = 0 or INT_MIN if it does not originate from a
	call i3.  Distribute must-not-throw REG_EH_REGION to all
	resulting instructions.
	(try_combine): Ensure that we can merge REG_EH_REGION notes.
	Ensure we are not splitting the non-call EH part of an insn.
	* except.h (insn_must_not_throw_p): Declare.
	* except.cc (insn_must_not_throw_p): New function.

	* gcc.dg/torture/pr105231.c: New testcase.
---
 gcc/combine.cc                          | 102 +++++++++++++++++++-----
 gcc/except.cc                           |  14 ++++
 gcc/except.h                            |   1 +
 gcc/testsuite/gcc.dg/torture/pr105231.c |  15 ++++
 4 files changed, 114 insertions(+), 18 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/torture/pr105231.c

diff --git a/gcc/combine.cc b/gcc/combine.cc
index 53dcac92abc..55a0fa58f48 100644
--- a/gcc/combine.cc
+++ b/gcc/combine.cc
@@ -91,6 +91,7 @@ along with GCC; see the file COPYING3.  If not see
 #include "print-rtl.h"
 #include "function-abi.h"
 #include "rtlanal.h"
+#include "except.h"
 
 /* Number of attempts to combine instructions in this function.  */
 
@@ -2951,6 +2952,31 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
       return 0;
     }
 
+  /* When i3 transfers to an EH handler we cannot combine if any of the
+     sources are within a must-not-throw region.  Else we can throw away
+     any nothrow, pick a random must-not-throw region or preserve the EH
+     transfer on i3.  Since we want to preserve nothrow notes on calls
+     we have to avoid combining from must-not-throw stmts there as well.
+     This has to be kept in sync with distribute_note.  */
+  if (rtx i3_eh = find_reg_note (i3, REG_EH_REGION, NULL_RTX))
+    {
+      int i3_lp_nr = INTVAL (XEXP (i3_eh, 0));
+      if (i3_lp_nr > 0
+	  || ((i3_lp_nr == 0 || i3_lp_nr == INT_MIN) && CALL_P (i3)))
+	{
+	  if (insn_must_not_throw_p (i2)
+	      || (i1 && insn_must_not_throw_p (i1))
+	      || (i0 && insn_must_not_throw_p (i0)))
+	    {
+	      if (dump_file && (dump_flags & TDF_DETAILS))
+		fprintf (dump_file, "Can't combine insn in must-not-throw "
+			 "EH region into i3 which can throws\n");
+	      undo_all ();
+	      return 0;
+	    }
+	}
+    }
+
   /* Record whether i2 and i3 are trivial moves.  */
   i2_was_move = is_just_move (i2);
   i3_was_move = is_just_move (i3);
@@ -3685,7 +3711,12 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
 	      || !modified_between_p (*split, i2, i3))
 	  /* We can't overwrite I2DEST if its value is still used by
 	     NEWPAT.  */
-	  && ! reg_referenced_p (i2dest, newpat))
+	  && ! reg_referenced_p (i2dest, newpat)
+	  /* We should not split a possibly trapping part of a throwing
+	     insn.  */
+	  && (!cfun->can_throw_non_call_exceptions
+	      || !can_throw_internal (i3)
+	      || !may_trap_p (*split)))
 	{
 	  rtx newdest = i2dest;
 	  enum rtx_code split_code = GET_CODE (*split);
@@ -14175,23 +14206,58 @@ distribute_notes (rtx notes, rtx_insn *from_insn, rtx_insn *i3, rtx_insn *i2,
 	  break;
 
 	case REG_EH_REGION:
-	  /* These notes must remain with the call or trapping instruction.  */
-	  if (CALL_P (i3))
-	    place = i3;
-	  else if (i2 && CALL_P (i2))
-	    place = i2;
-	  else
-	    {
-	      gcc_assert (cfun->can_throw_non_call_exceptions);
-	      if (may_trap_p (i3))
-		place = i3;
-	      else if (i2 && may_trap_p (i2))
-		place = i2;
-	      /* ??? Otherwise assume we've combined things such that we
-		 can now prove that the instructions can't trap.  Drop the
-		 note in this case.  */
-	    }
-	  break;
+	  {
+	    /* This handling needs to be kept in sync with the
+	       prerequisite checking in try_combine.  */
+	    int lp_nr = INTVAL (XEXP (note, 0));
+	    /* A REG_EH_REGION note transfering control can only ever come
+	       from i3 and it has to stay there.  */
+	    if (lp_nr > 0)
+	      {
+		gcc_assert (from_insn == i3);
+		if (CALL_P (i3))
+		  place = i3;
+		else
+		  {
+		    gcc_assert (cfun->can_throw_non_call_exceptions);
+		    /* If i3 can still trap preserve the note, otherwise we've
+		       combined things such that we can now prove that the
+		       instructions can't trap.  Drop the note in this case.  */
+		    if (may_trap_p (i3))
+		      place = i3;
+		  }
+		gcc_assert (!(cfun->can_throw_non_call_exceptions
+			      && i2 && may_trap_p (i2)));
+	      }
+	    else if (lp_nr == 0 || lp_nr == INT_MIN)
+	      {
+		/* nothrow notes can be dropped but we preserve
+		   those on calls.  */
+		if (from_insn == i3 && CALL_P (i3))
+		  place = i3;
+	      }
+	    else
+	      {
+		/* Distribute must-not-throw notes to all resulting
+		   insns and just pick the first.  */
+		if (!find_reg_note (i3, REG_EH_REGION, NULL_RTX)
+		    && (CALL_P (i3)
+			|| (cfun->can_throw_non_call_exceptions
+			    && may_trap_p (i3))))
+		  place = i3;
+		if (i2
+		    && !find_reg_note (i2, REG_EH_REGION, NULL_RTX)
+		    && cfun->can_throw_non_call_exceptions
+		    && may_trap_p (i2))
+		  {
+		    if (place)
+		      place2 = i2;
+		    else
+		      place = i2;
+		  }
+	      }
+	    break;
+	  }
 
 	case REG_ARGS_SIZE:
 	  /* ??? How to distribute between i3-i1.  Assume i3 contains the
diff --git a/gcc/except.cc b/gcc/except.cc
index b94de425557..4056f37840c 100644
--- a/gcc/except.cc
+++ b/gcc/except.cc
@@ -1927,6 +1927,20 @@ can_throw_external (const_rtx insn)
   return false;
 }
 
+/* Return true if INSN is associated with a must-not-throw EH region.  */
+
+bool
+insn_must_not_throw_p (rtx_insn *insn)
+{
+  if (rtx note = find_reg_note (insn, REG_EH_REGION, NULL_RTX))
+    {
+      int lp_nr = INTVAL (XEXP (note, 0));
+      if (lp_nr < 0 && lp_nr != INT_MIN)
+	return true;
+    }
+  return false;
+}
+
 /* Return true if INSN cannot throw at all.  */
 
 bool
diff --git a/gcc/except.h b/gcc/except.h
index b7fd0f419be..601f4f8cafb 100644
--- a/gcc/except.h
+++ b/gcc/except.h
@@ -280,6 +280,7 @@ extern void assign_filter_values (void);
 
 extern eh_region get_eh_region_from_rtx (const_rtx);
 extern eh_landing_pad get_eh_landing_pad_from_rtx (const_rtx);
+extern bool insn_must_not_throw_p (rtx_insn *);
 
 extern void finish_eh_generation (void);
 
diff --git a/gcc/testsuite/gcc.dg/torture/pr105231.c b/gcc/testsuite/gcc.dg/torture/pr105231.c
new file mode 100644
index 00000000000..50459219c08
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/torture/pr105231.c
@@ -0,0 +1,15 @@
+/* { dg-do compile } */
+/* { dg-require-effective-target int32plus } */
+/* { dg-require-effective-target dfp } */
+/* { dg-additional-options "-fsanitize-coverage=trace-pc -fnon-call-exceptions --param=max-cse-insns=1 -frounding-math" } */
+/* { dg-additional-options "-mstack-arg-probe" { target x86_64-*-* i?86-*-* } } */
+
+void baz (int *);
+void bar (double, double, _Decimal64);
+
+void
+foo (void)
+{
+  int s __attribute__((cleanup (baz)));
+  bar (0xfffffffffffffffe, 0xebf3fff2fbebaf7f, 0xffffffffffffff);
+}
  

On Thu, 21 Apr 2022, Richard Biener wrote:

> On Wed, 20 Apr 2022, Segher Boessenkool wrote:
> 
> > Hi!
> > 
> > This looks great :-)
> > 
> > On Wed, Apr 20, 2022 at 03:52:33PM +0200, Richard Biener wrote:
> > > The following mitigates a problem in combine distribute_notes which
> > > places an original REG_EH_REGION based on only may_trap_p which is
> > > good to test whether a non-call insn can possibly throw but not if
> > > actually it does or we care.  That's something we decided at RTL
> > > expansion time where we possibly still know the insn evaluates
> > > to a constant.
> > > 
> > > In fact, the REG_EH_REGION note with lp > 0 can only come from the
> > > original i3 and an assert is added to that effect.  That means we only
> > > need to retain the note on i3 or, if that cannot trap, drop it but we
> > > should never move it to i2.
> > > 
> > > For REG_EH_REGION corresponding to must-not-throw regions or
> > > nothrow marking try_combine gets new code ensuring we can merge
> > > and distribute notes which means placing must-not-throw notes
> > > on all result insns, and dropping nothrow notes or preserve
> > > them on i3 for calls.
> > 
> > > 	* combine.cc (distribute_notes): Assert that a REG_EH_REGION
> > > 	with landing pad > 0 is from i3 and only keep it there or drop
> > > 	it if the insn can not trap.  Throw away REG_EH_REGION with
> > > 	landing pad = 0 or INT_MIN if it does not originate from a
> > > 	call i3.  Distribute must-not-throw REG_EH_REGION to all
> > > 	resulting instructions.
> > > 	(try_combine): Ensure that we can merge REG_EH_REGION notes.
> > 
> > > --- a/gcc/combine.cc
> > > +++ b/gcc/combine.cc
> > > @@ -2951,6 +2951,45 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
> > >        return 0;
> > >      }
> > >  
> > > +  /* When i3 transfers to an EH handler we cannot combine if any of the
> > > +     sources are within a must-not-throw region.  Else we can throw away
> > > +     any nothrow, pick a random must-not-throw region or preserve the EH
> > > +     transfer on i3.  Since we want to preserve nothrow notes on calls
> > > +     we have to avoid combining from must-not-throw stmts there as well.
> > > +     This has to be kept in sync with distribute_note.  */
> > > +  if (rtx i3_eh = find_reg_note (i3, REG_EH_REGION, NULL_RTX))
> > > +    {
> > > +      int i3_lp_nr = INTVAL (XEXP (i3_eh, 0));
> > > +      if (i3_lp_nr > 0
> > > +	  || ((i3_lp_nr == 0 || i3_lp_nr == INT_MIN) && CALL_P (i3)))
> > > +	{
> > > +	  rtx eh;
> > > +	  int eh_lp;
> > > +	  if (((eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > > +	       && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > +	       && eh_lp != INT_MIN)
> > > +	      || (i2
> > > +		  && (eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > +		  && eh_lp != INT_MIN)
> > > +	      || (i1
> > > +		  && (eh = find_reg_note (i1, REG_EH_REGION, NULL_RTX))
> > > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > +		  && eh_lp != INT_MIN)
> > > +	      || (i0
> > > +		  && (eh = find_reg_note (i0, REG_EH_REGION, NULL_RTX))
> > > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > +		  && eh_lp != INT_MIN))
> > > +	    {
> > > +	      if (dump_file && (dump_flags & TDF_DETAILS))
> > > +		fprintf (dump_file, "Can't combine insn in must-not-throw "
> > > +			 "EH region into i3 which can throws\n");
> > > +	      undo_all ();
> > > +	      return 0;
> > > +	    }
> > > +	}
> > > +    }
> > 
> > The assignments in the conditionals make this hard to read, and harder
> > to change, btw.  A utility function wouldn't hurt?  The problem of
> > course would be thinking of a good name for it :-)
> 
> I've added insn_must_not_throw_p to except.{cc,h}.
> 
> > >  	case REG_EH_REGION:
> > > -	  /* These notes must remain with the call or trapping instruction.  */
> > > -	  if (CALL_P (i3))
> > > -	    place = i3;
> > > -	  else if (i2 && CALL_P (i2))
> > > -	    place = i2;
> > > -	  else
> > > -	    {
> > > -	      gcc_assert (cfun->can_throw_non_call_exceptions);
> > > -	      if (may_trap_p (i3))
> > > -		place = i3;
> > > -	      else if (i2 && may_trap_p (i2))
> > > -		place = i2;
> > > -	      /* ??? Otherwise assume we've combined things such that we
> > > -		 can now prove that the instructions can't trap.  Drop the
> > > -		 note in this case.  */
> > > -	    }
> > > -	  break;
> > > +	  {
> > > +	    /* This handling needs to be kept in sync with the
> > > +	       prerequesite checking in try_combine.  */
> > 
> > (prerequisite)
> 
> Fixed.
> 
> > > +	    int lp_nr = INTVAL (XEXP (note, 0));
> > > +	    /* A REG_EH_REGION note transfering control can only ever come
> > > +	       from i3 and it has to stay there.  */
> > > +	    if (lp_nr > 0)
> > > +	      {
> > > +		gcc_assert (from_insn == i3);
> > > +		if (CALL_P (i3))
> > > +		  place = i3;
> > > +		else
> > > +		  {
> > > +		    gcc_assert (cfun->can_throw_non_call_exceptions);
> > > +		    /* If i3 can still trap preserve the note, otherwise we've
> > > +		       combined things such that we can now prove that the
> > > +		       instructions can't trap.  Drop the note in this case.  */
> > > +		    if (may_trap_p (i3))
> > > +		      place = i3;
> > > +		  }
> > > +	      }
> > 
> > It isn't immediately clear to me that we cannot have moved a trapping
> > thing to i2?  Do we guarantee that somewhere?  Can we / should we assert
> > that here?  gcc_assert (!(i2 && may_trap_p (i2)))  or something like
> > that.
> 
> IMHO asserts for catching things we don't handle are bad.  I suppose
> I could add sth like
> 
> @@ -3724,7 +3712,12 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn 
> *i1, rt
> x_insn *i0,
>               || !modified_between_p (*split, i2, i3))
>           /* We can't overwrite I2DEST if its value is still used by
>              NEWPAT.  */
> -         && ! reg_referenced_p (i2dest, newpat))
> +         && ! reg_referenced_p (i2dest, newpat)
> +         /* We should not split a possibly trapping part of a throwing
> +            insn.  */
> +         && (!cfun->can_throw_non_call_exceptions
> +             || !can_throw_internal (i3)
> +             || !may_trap_p (*split)))
>         {
>           rtx newdest = i2dest;
>           enum rtx_code split_code = GET_CODE (*split);
> 
> but then without a testcase it's hard to see if this is to the point.
> It of course also assumes we're never going to split the call part
> from i3 to i2.
> 
> It should be at least safe and with that we could add your assert,
> sligthly altered to
> 
>                 gcc_assert (!(cfun->can_throw_non_call_exceptions
>                               && i2 && may_trap_p (i2)));
> 
> because we have to guard against spltting say a trapping memory
> load out from a CALL_P, but only with non-call EH (the call
> might not throw but a (pre-existing!) memory load could trap).

So that doesn't work, it ICEs

FAIL:   c52103k
FAIL:   c52103p
FAIL:   c52104p
FAIL:   c95089a
FAIL:   cxa4034

that means the

      /* If we can split it and use I2DEST, go ahead and see if that
         helps things be recognized.  Verify that none of the registers
         are set between I2 and I3.  */
      if (insn_code_number < 0
          && (split = find_split_point (&newpat, i3, false)) != 0
...

case isn't the only place we can split i2 (as seen in 
distribute_notes) from i3?

This is for example

Trying 901, 902 -> 903:
  901: r556:DI=0x9
  902: {r555:DI=r556:DI-r132:DI;clobber flags:CC;}
      REG_DEAD r556:DI
      REG_DEAD r132:DI
      REG_UNUSED flags:CC
      REG_EQUAL 0x9-r132:DI
  903: r256:QI=[r234:DI+r555:DI]
      REG_DEAD r555:DI
      REG_DEAD r234:DI
      REG_EH_REGION 0x9

Successfully matched this instruction:
(set (reg:DI 555)
    (minus:DI (reg/f:DI 234 [ arrx52.69 ])
        (reg:DI 132 [ _58 ])))
Successfully matched this instruction:
(set (reg:QI 256 [ _320 ])
    (mem/j:QI (plus:DI (reg:DI 555)
            (const_int 9 [0x9])) [11 (*arrx52.69_259)[9]{lb: _58 sz: 1}+0 
S1 A8]))

where it's probably overzealeous, and just because we have at
this point

(insn 902 901 903 134 (parallel [
            (set (reg:DI 555)
                (minus:DI (reg/f:DI 234 [ arrx52.69 ])
                    (reg:DI 132 [ _58 ])))
            (clobber (reg:CC 17 flags))
        ]) "c52104p.adb":273:44 303 {*subdi_1}
     (expr_list:REG_DEAD (reg/f:DI 234 [ arrx52.69 ])
        (nil)))

and may_trap_p does

    case EXPR_LIST:
      /* An EXPR_LIST is used to represent a function call.  This
         certainly may trap.  */
      return 1;

so in this case for -fnon-call-exceptions we are going to reject
a lot of splits - but we actually don't (because the may_trap_p
check on the split point fails).  I suppose for the assert
we'd rather want to check may_trap_p (PATTERN (i2)) then?
Do we want to do this for the other (non-call?) cases as well?

Changing the assert to

                gcc_assert (!(cfun->can_throw_non_call_exceptions
                              && i2 && may_trap_p (PATTERN (i2))));

fixes the ICE (just checked one of the testcases).

I'm going to re-check with this, but given may_trap_p is looking
to err on the trapping side too easily maybe an assert isn't a good
idea.

Richard.
  

On Thu, 21 Apr 2022, Richard Biener wrote:

> On Thu, 21 Apr 2022, Richard Biener wrote:
> 
> > On Wed, 20 Apr 2022, Segher Boessenkool wrote:
> > 
> > > Hi!
> > > 
> > > This looks great :-)
> > > 
> > > On Wed, Apr 20, 2022 at 03:52:33PM +0200, Richard Biener wrote:
> > > > The following mitigates a problem in combine distribute_notes which
> > > > places an original REG_EH_REGION based on only may_trap_p which is
> > > > good to test whether a non-call insn can possibly throw but not if
> > > > actually it does or we care.  That's something we decided at RTL
> > > > expansion time where we possibly still know the insn evaluates
> > > > to a constant.
> > > > 
> > > > In fact, the REG_EH_REGION note with lp > 0 can only come from the
> > > > original i3 and an assert is added to that effect.  That means we only
> > > > need to retain the note on i3 or, if that cannot trap, drop it but we
> > > > should never move it to i2.
> > > > 
> > > > For REG_EH_REGION corresponding to must-not-throw regions or
> > > > nothrow marking try_combine gets new code ensuring we can merge
> > > > and distribute notes which means placing must-not-throw notes
> > > > on all result insns, and dropping nothrow notes or preserve
> > > > them on i3 for calls.
> > > 
> > > > 	* combine.cc (distribute_notes): Assert that a REG_EH_REGION
> > > > 	with landing pad > 0 is from i3 and only keep it there or drop
> > > > 	it if the insn can not trap.  Throw away REG_EH_REGION with
> > > > 	landing pad = 0 or INT_MIN if it does not originate from a
> > > > 	call i3.  Distribute must-not-throw REG_EH_REGION to all
> > > > 	resulting instructions.
> > > > 	(try_combine): Ensure that we can merge REG_EH_REGION notes.
> > > 
> > > > --- a/gcc/combine.cc
> > > > +++ b/gcc/combine.cc
> > > > @@ -2951,6 +2951,45 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn *i1, rtx_insn *i0,
> > > >        return 0;
> > > >      }
> > > >  
> > > > +  /* When i3 transfers to an EH handler we cannot combine if any of the
> > > > +     sources are within a must-not-throw region.  Else we can throw away
> > > > +     any nothrow, pick a random must-not-throw region or preserve the EH
> > > > +     transfer on i3.  Since we want to preserve nothrow notes on calls
> > > > +     we have to avoid combining from must-not-throw stmts there as well.
> > > > +     This has to be kept in sync with distribute_note.  */
> > > > +  if (rtx i3_eh = find_reg_note (i3, REG_EH_REGION, NULL_RTX))
> > > > +    {
> > > > +      int i3_lp_nr = INTVAL (XEXP (i3_eh, 0));
> > > > +      if (i3_lp_nr > 0
> > > > +	  || ((i3_lp_nr == 0 || i3_lp_nr == INT_MIN) && CALL_P (i3)))
> > > > +	{
> > > > +	  rtx eh;
> > > > +	  int eh_lp;
> > > > +	  if (((eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > > > +	       && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > > +	       && eh_lp != INT_MIN)
> > > > +	      || (i2
> > > > +		  && (eh = find_reg_note (i2, REG_EH_REGION, NULL_RTX))
> > > > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > > +		  && eh_lp != INT_MIN)
> > > > +	      || (i1
> > > > +		  && (eh = find_reg_note (i1, REG_EH_REGION, NULL_RTX))
> > > > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > > +		  && eh_lp != INT_MIN)
> > > > +	      || (i0
> > > > +		  && (eh = find_reg_note (i0, REG_EH_REGION, NULL_RTX))
> > > > +		  && (eh_lp = INTVAL (XEXP (eh, 0))) < 0
> > > > +		  && eh_lp != INT_MIN))
> > > > +	    {
> > > > +	      if (dump_file && (dump_flags & TDF_DETAILS))
> > > > +		fprintf (dump_file, "Can't combine insn in must-not-throw "
> > > > +			 "EH region into i3 which can throws\n");
> > > > +	      undo_all ();
> > > > +	      return 0;
> > > > +	    }
> > > > +	}
> > > > +    }
> > > 
> > > The assignments in the conditionals make this hard to read, and harder
> > > to change, btw.  A utility function wouldn't hurt?  The problem of
> > > course would be thinking of a good name for it :-)
> > 
> > I've added insn_must_not_throw_p to except.{cc,h}.
> > 
> > > >  	case REG_EH_REGION:
> > > > -	  /* These notes must remain with the call or trapping instruction.  */
> > > > -	  if (CALL_P (i3))
> > > > -	    place = i3;
> > > > -	  else if (i2 && CALL_P (i2))
> > > > -	    place = i2;
> > > > -	  else
> > > > -	    {
> > > > -	      gcc_assert (cfun->can_throw_non_call_exceptions);
> > > > -	      if (may_trap_p (i3))
> > > > -		place = i3;
> > > > -	      else if (i2 && may_trap_p (i2))
> > > > -		place = i2;
> > > > -	      /* ??? Otherwise assume we've combined things such that we
> > > > -		 can now prove that the instructions can't trap.  Drop the
> > > > -		 note in this case.  */
> > > > -	    }
> > > > -	  break;
> > > > +	  {
> > > > +	    /* This handling needs to be kept in sync with the
> > > > +	       prerequesite checking in try_combine.  */
> > > 
> > > (prerequisite)
> > 
> > Fixed.
> > 
> > > > +	    int lp_nr = INTVAL (XEXP (note, 0));
> > > > +	    /* A REG_EH_REGION note transfering control can only ever come
> > > > +	       from i3 and it has to stay there.  */
> > > > +	    if (lp_nr > 0)
> > > > +	      {
> > > > +		gcc_assert (from_insn == i3);
> > > > +		if (CALL_P (i3))
> > > > +		  place = i3;
> > > > +		else
> > > > +		  {
> > > > +		    gcc_assert (cfun->can_throw_non_call_exceptions);
> > > > +		    /* If i3 can still trap preserve the note, otherwise we've
> > > > +		       combined things such that we can now prove that the
> > > > +		       instructions can't trap.  Drop the note in this case.  */
> > > > +		    if (may_trap_p (i3))
> > > > +		      place = i3;
> > > > +		  }
> > > > +	      }
> > > 
> > > It isn't immediately clear to me that we cannot have moved a trapping
> > > thing to i2?  Do we guarantee that somewhere?  Can we / should we assert
> > > that here?  gcc_assert (!(i2 && may_trap_p (i2)))  or something like
> > > that.
> > 
> > IMHO asserts for catching things we don't handle are bad.  I suppose
> > I could add sth like
> > 
> > @@ -3724,7 +3712,12 @@ try_combine (rtx_insn *i3, rtx_insn *i2, rtx_insn 
> > *i1, rt
> > x_insn *i0,
> >               || !modified_between_p (*split, i2, i3))
> >           /* We can't overwrite I2DEST if its value is still used by
> >              NEWPAT.  */
> > -         && ! reg_referenced_p (i2dest, newpat))
> > +         && ! reg_referenced_p (i2dest, newpat)
> > +         /* We should not split a possibly trapping part of a throwing
> > +            insn.  */
> > +         && (!cfun->can_throw_non_call_exceptions
> > +             || !can_throw_internal (i3)
> > +             || !may_trap_p (*split)))
> >         {
> >           rtx newdest = i2dest;
> >           enum rtx_code split_code = GET_CODE (*split);
> > 
> > but then without a testcase it's hard to see if this is to the point.
> > It of course also assumes we're never going to split the call part
> > from i3 to i2.
> > 
> > It should be at least safe and with that we could add your assert,
> > sligthly altered to
> > 
> >                 gcc_assert (!(cfun->can_throw_non_call_exceptions
> >                               && i2 && may_trap_p (i2)));
> > 
> > because we have to guard against spltting say a trapping memory
> > load out from a CALL_P, but only with non-call EH (the call
> > might not throw but a (pre-existing!) memory load could trap).
> 
> So that doesn't work, it ICEs
> 
> FAIL:   c52103k
> FAIL:   c52103p
> FAIL:   c52104p
> FAIL:   c95089a
> FAIL:   cxa4034
> 
> that means the
> 
>       /* If we can split it and use I2DEST, go ahead and see if that
>          helps things be recognized.  Verify that none of the registers
>          are set between I2 and I3.  */
>       if (insn_code_number < 0
>           && (split = find_split_point (&newpat, i3, false)) != 0
> ...
> 
> case isn't the only place we can split i2 (as seen in 
> distribute_notes) from i3?
> 
> This is for example
> 
> Trying 901, 902 -> 903:
>   901: r556:DI=0x9
>   902: {r555:DI=r556:DI-r132:DI;clobber flags:CC;}
>       REG_DEAD r556:DI
>       REG_DEAD r132:DI
>       REG_UNUSED flags:CC
>       REG_EQUAL 0x9-r132:DI
>   903: r256:QI=[r234:DI+r555:DI]
>       REG_DEAD r555:DI
>       REG_DEAD r234:DI
>       REG_EH_REGION 0x9
> 
> Successfully matched this instruction:
> (set (reg:DI 555)
>     (minus:DI (reg/f:DI 234 [ arrx52.69 ])
>         (reg:DI 132 [ _58 ])))
> Successfully matched this instruction:
> (set (reg:QI 256 [ _320 ])
>     (mem/j:QI (plus:DI (reg:DI 555)
>             (const_int 9 [0x9])) [11 (*arrx52.69_259)[9]{lb: _58 sz: 1}+0 
> S1 A8]))
> 
> where it's probably overzealeous, and just because we have at
> this point
> 
> (insn 902 901 903 134 (parallel [
>             (set (reg:DI 555)
>                 (minus:DI (reg/f:DI 234 [ arrx52.69 ])
>                     (reg:DI 132 [ _58 ])))
>             (clobber (reg:CC 17 flags))
>         ]) "c52104p.adb":273:44 303 {*subdi_1}
>      (expr_list:REG_DEAD (reg/f:DI 234 [ arrx52.69 ])
>         (nil)))
> 
> and may_trap_p does
> 
>     case EXPR_LIST:
>       /* An EXPR_LIST is used to represent a function call.  This
>          certainly may trap.  */
>       return 1;
> 
> so in this case for -fnon-call-exceptions we are going to reject
> a lot of splits - but we actually don't (because the may_trap_p
> check on the split point fails).  I suppose for the assert
> we'd rather want to check may_trap_p (PATTERN (i2)) then?
> Do we want to do this for the other (non-call?) cases as well?
> 
> Changing the assert to
> 
>                 gcc_assert (!(cfun->can_throw_non_call_exceptions
>                               && i2 && may_trap_p (PATTERN (i2))));
> 
> fixes the ICE (just checked one of the testcases).
> 
> I'm going to re-check with this, but given may_trap_p is looking
> to err on the trapping side too easily maybe an assert isn't a good
> idea.

With the adjusted assert the patch passes bootstrap & regtest on 
x86_64-unknown-linux-gnu.

We might be still doing things horribly wrong WRT non-call exceptions
and EH (esp. when must-not-throw is involved, but not necessarily only
with that).  Test coverage is likely zero here (we got away with being
lazy here until the PR with the artificial testcase and set of options
came along).

OK for trunk?  I personally prefer to drop the assert (esp. at this
point) though.

Thanks,
Richard.