Re: [PATCH][v3] rtl-optimization/105231 - distribute_notes and REG_EH_REGION

[Richard Biener <rguenther@suse.de>]

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.