[2/3] tree-cfg: do not duplicate returns_twice calls

  A returns_twice call may have associated abnormal edges that correspond
to the "second return" from the call. If the call is duplicated, the
copies of those edges also need to be abnormal, but e.g. tracer does not
enforce that. Just prohibit the (unlikely to be useful) duplication.

gcc/ChangeLog:

	* tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
	calls that may return twice.
---
 gcc/tree-cfg.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
  

On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> A returns_twice call may have associated abnormal edges that correspond
> to the "second return" from the call. If the call is duplicated, the
> copies of those edges also need to be abnormal, but e.g. tracer does not
> enforce that. Just prohibit the (unlikely to be useful) duplication.

The general CFG copying routines properly duplicate those edges, no?
Tracer uses duplicate_block so it should also get copies of all successor
edges of that block.  It also only traces along normal edges.  What it might
miss is abnormal incoming edges - is that what you are referring to?

That would be a thing we don't handle in duplicate_block on its own but
that callers are expected to do (though I don't see copy_bbs doing that
either).  I wonder if we can trigger this issue for some testcase?

The thing to check would be incoming abnormal edges in
can_duplicate_block_p, not (only) returns twice functions?

Richard.

> gcc/ChangeLog:
>
>         * tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
>         calls that may return twice.
> ---
>  gcc/tree-cfg.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index b7fe313b7..a99f1acb4 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
>      {
>        gimple *g = gsi_stmt (gsi);
>
> -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> +      /* Prohibit duplication of returns_twice calls, otherwise associated
> +        abnormal edges also need to be duplicated properly.
> +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
>          duplicated as part of its group, or not at all.
>          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
>          group, so the same holds there.  */
>        if (is_gimple_call (g)
> -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> --
> 2.33.1
>
  

Apologies for the prolonged silence Richard, it is a bit of an obscure topic,
and I was unsure I'd be able to handle any complications in a timely manner.
I'm ready to revisit it now, please see below.

On Mon, 17 Jan 2022, Richard Biener wrote:

> On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> > A returns_twice call may have associated abnormal edges that correspond
> > to the "second return" from the call. If the call is duplicated, the
> > copies of those edges also need to be abnormal, but e.g. tracer does not
> > enforce that. Just prohibit the (unlikely to be useful) duplication.
> 
> The general CFG copying routines properly duplicate those edges, no?

No (in fact you say so in the next paragraph). In general I think they cannot,
abnormal edges are a special case, so it should be the responsibility of the
caller.

> Tracer uses duplicate_block so it should also get copies of all successor
> edges of that block.  It also only traces along normal edges.  What it might
> miss is abnormal incoming edges - is that what you are referring to?

Yes (I think its entire point is to build a "trace" of duplicated blocks that
does not have incoming edges in the middle, abnormal or not).

> That would be a thing we don't handle in duplicate_block on its own but
> that callers are expected to do (though I don't see copy_bbs doing that
> either).  I wonder if we can trigger this issue for some testcase?

Oh yes (in fact my desire to find a testcase delayed this quite a bit).
When compiling the following testcase with -O2 -ftracer:

__attribute__((returns_twice))
int rtwice_a(int), rtwice_b(int);

int f(int *x)
{
        volatile unsigned k, i = (*x);

        for (k = 1; (i = rtwice_a(i)) * k; k = 2);

        for (; (i = rtwice_b(i)) * k; k = 4);

        return k;
}

tracer manages to eliminate the ABNORMAL_DISPATCHER block completely, so
the possibility of transferring control back to rtwice_a from rtwice_b
is no longer modeled in the IR. I could spend some time "upgrading" this
to an end-to-end miscompilation, but I hope you agree this is quite broken
already.

> The thing to check would be incoming abnormal edges in
> can_duplicate_block_p, not (only) returns twice functions?

Unfortunately not, abnormal edges are also used for computed gotos, which are
less magic than returns_twice edges and should not block tracer I think.

This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
[1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html

How would you like to proceed here? Is my initial patch ok?

Alexander

> 
> Richard.
> 
> > gcc/ChangeLog:
> >
> >         * tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
> >         calls that may return twice.
> > ---
> >  gcc/tree-cfg.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > index b7fe313b7..a99f1acb4 100644
> > --- a/gcc/tree-cfg.c
> > +++ b/gcc/tree-cfg.c
> > @@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
> >      {
> >        gimple *g = gsi_stmt (gsi);
> >
> > -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> > +      /* Prohibit duplication of returns_twice calls, otherwise associated
> > +        abnormal edges also need to be duplicated properly.
> > +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> >          duplicated as part of its group, or not at all.
> >          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
> >          group, so the same holds there.  */
> >        if (is_gimple_call (g)
> > -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> > +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> > +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
> >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
> >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> > --
> > 2.33.1
> >
>
  

On Tue, Jul 12, 2022 at 10:10 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> Apologies for the prolonged silence Richard, it is a bit of an obscure topic,
> and I was unsure I'd be able to handle any complications in a timely manner.
> I'm ready to revisit it now, please see below.
>
> On Mon, 17 Jan 2022, Richard Biener wrote:
>
> > On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
> > >
> > > A returns_twice call may have associated abnormal edges that correspond
> > > to the "second return" from the call. If the call is duplicated, the
> > > copies of those edges also need to be abnormal, but e.g. tracer does not
> > > enforce that. Just prohibit the (unlikely to be useful) duplication.
> >
> > The general CFG copying routines properly duplicate those edges, no?
>
> No (in fact you say so in the next paragraph). In general I think they cannot,
> abnormal edges are a special case, so it should be the responsibility of the
> caller.
>
> > Tracer uses duplicate_block so it should also get copies of all successor
> > edges of that block.  It also only traces along normal edges.  What it might
> > miss is abnormal incoming edges - is that what you are referring to?
>
> Yes (I think its entire point is to build a "trace" of duplicated blocks that
> does not have incoming edges in the middle, abnormal or not).
>
> > That would be a thing we don't handle in duplicate_block on its own but
> > that callers are expected to do (though I don't see copy_bbs doing that
> > either).  I wonder if we can trigger this issue for some testcase?
>
> Oh yes (in fact my desire to find a testcase delayed this quite a bit).
> When compiling the following testcase with -O2 -ftracer:
>
> __attribute__((returns_twice))
> int rtwice_a(int), rtwice_b(int);
>
> int f(int *x)
> {
>         volatile unsigned k, i = (*x);
>
>         for (k = 1; (i = rtwice_a(i)) * k; k = 2);
>
>         for (; (i = rtwice_b(i)) * k; k = 4);
>
>         return k;
> }
>
> tracer manages to eliminate the ABNORMAL_DISPATCHER block completely, so
> the possibility of transferring control back to rtwice_a from rtwice_b
> is no longer modeled in the IR. I could spend some time "upgrading" this
> to an end-to-end miscompilation, but I hope you agree this is quite broken
> already.
>
> > The thing to check would be incoming abnormal edges in
> > can_duplicate_block_p, not (only) returns twice functions?
>
> Unfortunately not, abnormal edges are also used for computed gotos, which are
> less magic than returns_twice edges and should not block tracer I think.

I think computed gotos should use regular edges, only non-local goto should
use abnormals...

I suppose asm goto also uses abnormal edges?

Btw, I don't see how they in general are "less magic".  Sure, we have an
explicit receiver (the destination label), but we can only do edge inserts
if we have a single computed goto edge into a block (we can "move" the
label to the block created when splitting the edge).

> This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
> [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html
>
> How would you like to proceed here? Is my initial patch ok?

Hmm, so for returns twice calls duplicate_block correctly copies the call
and redirects the provided incoming edge to it.  The API does not
handle adding any further incoming edges - the caller would be responsible
for this.  So I still somewhat fail to see the point here.  If tracer does not
handle extra incoming edges properly then we need to fix tracer?  This
also includes non-local goto (we seem to copy non-local labels just
fine - wasn't there a bugreport about this!?).

So I think can_duplicate_block_p is the wrong place to fix (the RTL side
would need a similar fix anyhow?)

Richard.

> Alexander
>
> >
> > Richard.
> >
> > > gcc/ChangeLog:
> > >
> > >         * tree-cfg.c (gimple_can_duplicate_bb_p): Reject blocks with
> > >         calls that may return twice.
> > > ---
> > >  gcc/tree-cfg.c | 7 +++++--
> > >  1 file changed, 5 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> > > index b7fe313b7..a99f1acb4 100644
> > > --- a/gcc/tree-cfg.c
> > > +++ b/gcc/tree-cfg.c
> > > @@ -6304,12 +6304,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
> > >      {
> > >        gimple *g = gsi_stmt (gsi);
> > >
> > > -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> > > +      /* Prohibit duplication of returns_twice calls, otherwise associated
> > > +        abnormal edges also need to be duplicated properly.
> > > +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> > >          duplicated as part of its group, or not at all.
> > >          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
> > >          group, so the same holds there.  */
> > >        if (is_gimple_call (g)
> > > -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> > > +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> > > +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> > >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
> > >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
> > >               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> > > --
> > > 2.33.1
> > >
> >
  

On Wed, 13 Jul 2022, Richard Biener wrote:

> > > The thing to check would be incoming abnormal edges in
> > > can_duplicate_block_p, not (only) returns twice functions?
> >
> > Unfortunately not, abnormal edges are also used for computed gotos, which are
> > less magic than returns_twice edges and should not block tracer I think.
> 
> I think computed gotos should use regular edges, only non-local goto should
> use abnormals...

Yeah, afaict it's not documented what "abnormal" is supposed to mean :/

> I suppose asm goto also uses abnormal edges?

Heh, no, asm goto appears to use normal edges, but there's an old gap in
their specification: can you use them like computed gotos, i.e. can asm-goto
jump to a computed target? Or must they be similar to plain gotos where the
jump label is redirectable (because it's substitutable in the asm template)?

If you take a restrictive interpretation (asm goto may not jump to a computed
label) then using regular edges looks fine.

> Btw, I don't see how they in general are "less magic".  Sure, we have an
> explicit receiver (the destination label), but we can only do edge inserts
> if we have a single computed goto edge into a block (we can "move" the
> label to the block created when splitting the edge).

Sure, they are a bit magic, but returns_twice edges are even more magic: their
destination looks tied to a label in the IR, but in reality their destination
is inside a call that returns twice (hence GCC must be careful not to insert
anything between the label and the call, like in patch 1/3).

> > This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
> > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html
> >
> > How would you like to proceed here? Is my initial patch ok?
> 
> Hmm, so for returns twice calls duplicate_block correctly copies the call
> and redirects the provided incoming edge to it.  The API does not
> handle adding any further incoming edges - the caller would be responsible
> for this.  So I still somewhat fail to see the point here.  If tracer does not
> handle extra incoming edges properly then we need to fix tracer?

I think abnormal edges corresponding to computed gotos are fine: we are
attempting to create a chain of blocks with no incoming edges in the middle,
right? Destinations of computed gotos remain at labels of original blocks.

Agreed about correcting this in the tracer.

> This also includes non-local goto (we seem to copy non-local labels just
> fine - wasn't there a bugreport about this!?).

Sorry, no idea about this.

> So I think can_duplicate_block_p is the wrong place to fix (the RTL side
> would need a similar fix anyhow?)

Right. I'm happy to leave both RTL and GIMPLE can_duplicate_block_p as is,
and instead constrain just the tracer. Alternative patch below:

        * tracer.cc (analyze_bb): Disallow duplication of returns_twice calls.

diff --git a/gcc/tracer.cc b/gcc/tracer.cc
index 64517846d..422e2b6a7 100644
--- a/gcc/tracer.cc
+++ b/gcc/tracer.cc
@@ -132,14 +132,19 @@ analyze_bb (basic_block bb, int *count)
   gimple *stmt;
   int n = 0;

+  bool can_dup = can_duplicate_block_p (CONST_CAST_BB (bb));
+
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
       stmt = gsi_stmt (gsi);
       n += estimate_num_insns (stmt, &eni_size_weights);
+      if (can_dup && cfun->calls_setjmp && gimple_code (stmt) == GIMPLE_CALL
+         && gimple_call_flags (stmt) & ECF_RETURNS_TWICE)
+       can_dup = false;
     }
   *count = n;

-  cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb)));
+  cache_can_duplicate_bb_p (bb, can_dup);
 }

 /* Return true if E1 is more frequent than E2.  */
  

On 7/13/2022 1:13 AM, Richard Biener via Gcc-patches wrote:
> On Tue, Jul 12, 2022 at 10:10 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>>
>> Apologies for the prolonged silence Richard, it is a bit of an obscure topic,
>> and I was unsure I'd be able to handle any complications in a timely manner.
>> I'm ready to revisit it now, please see below.
>>
>> On Mon, 17 Jan 2022, Richard Biener wrote:
>>
>>> On Fri, Jan 14, 2022 at 7:21 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>>>> A returns_twice call may have associated abnormal edges that correspond
>>>> to the "second return" from the call. If the call is duplicated, the
>>>> copies of those edges also need to be abnormal, but e.g. tracer does not
>>>> enforce that. Just prohibit the (unlikely to be useful) duplication.
>>> The general CFG copying routines properly duplicate those edges, no?
>> No (in fact you say so in the next paragraph). In general I think they cannot,
>> abnormal edges are a special case, so it should be the responsibility of the
>> caller.
>>
>>> Tracer uses duplicate_block so it should also get copies of all successor
>>> edges of that block.  It also only traces along normal edges.  What it might
>>> miss is abnormal incoming edges - is that what you are referring to?
>> Yes (I think its entire point is to build a "trace" of duplicated blocks that
>> does not have incoming edges in the middle, abnormal or not).
>>
>>> That would be a thing we don't handle in duplicate_block on its own but
>>> that callers are expected to do (though I don't see copy_bbs doing that
>>> either).  I wonder if we can trigger this issue for some testcase?
>> Oh yes (in fact my desire to find a testcase delayed this quite a bit).
>> When compiling the following testcase with -O2 -ftracer:
>>
>> __attribute__((returns_twice))
>> int rtwice_a(int), rtwice_b(int);
>>
>> int f(int *x)
>> {
>>          volatile unsigned k, i = (*x);
>>
>>          for (k = 1; (i = rtwice_a(i)) * k; k = 2);
>>
>>          for (; (i = rtwice_b(i)) * k; k = 4);
>>
>>          return k;
>> }
>>
>> tracer manages to eliminate the ABNORMAL_DISPATCHER block completely, so
>> the possibility of transferring control back to rtwice_a from rtwice_b
>> is no longer modeled in the IR. I could spend some time "upgrading" this
>> to an end-to-end miscompilation, but I hope you agree this is quite broken
>> already.
>>
>>> The thing to check would be incoming abnormal edges in
>>> can_duplicate_block_p, not (only) returns twice functions?
>> Unfortunately not, abnormal edges are also used for computed gotos, which are
>> less magic than returns_twice edges and should not block tracer I think.
> I think computed gotos should use regular edges, only non-local goto should
> use abnormals...
>
> I suppose asm goto also uses abnormal edges?
>
> Btw, I don't see how they in general are "less magic".  Sure, we have an
> explicit receiver (the destination label), but we can only do edge inserts
> if we have a single computed goto edge into a block (we can "move" the
> label to the block created when splitting the edge).
I suspect treating them like abnormals probably came from the inability 
to reliably split them way back when we introduced RTL GCSE and the like.

Jeff
  

On Wed, Jul 13, 2022 at 4:57 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> On Wed, 13 Jul 2022, Richard Biener wrote:
>
> > > > The thing to check would be incoming abnormal edges in
> > > > can_duplicate_block_p, not (only) returns twice functions?
> > >
> > > Unfortunately not, abnormal edges are also used for computed gotos, which are
> > > less magic than returns_twice edges and should not block tracer I think.
> >
> > I think computed gotos should use regular edges, only non-local goto should
> > use abnormals...
>
> Yeah, afaict it's not documented what "abnormal" is supposed to mean :/
>
> > I suppose asm goto also uses abnormal edges?
>
> Heh, no, asm goto appears to use normal edges, but there's an old gap in
> their specification: can you use them like computed gotos, i.e. can asm-goto
> jump to a computed target? Or must they be similar to plain gotos where the
> jump label is redirectable (because it's substitutable in the asm template)?
>
> If you take a restrictive interpretation (asm goto may not jump to a computed
> label) then using regular edges looks fine.
>
> > Btw, I don't see how they in general are "less magic".  Sure, we have an
> > explicit receiver (the destination label), but we can only do edge inserts
> > if we have a single computed goto edge into a block (we can "move" the
> > label to the block created when splitting the edge).
>
> Sure, they are a bit magic, but returns_twice edges are even more magic: their
> destination looks tied to a label in the IR, but in reality their destination
> is inside a call that returns twice (hence GCC must be careful not to insert
> anything between the label and the call, like in patch 1/3).

Indeed.  Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right".

When copying a block we do not copy labels so any "jumps" remain to the original
block and thus we are indeed able to isolate normal control flow.  Given that
returns_twice functions _do_ seem to be special, and also special as to how
we handle other abnormal receivers in duplicate_block.  So it might indeed make
sense to special-case them in can_duplicate_block_p ... (sorry for
going back-and-forth ...)

Note that I think this detail of duplicate_block (the function) and the hook
needs to be better documented (the semantics on incoming edges, not duplicating
labels used for incoming control flow).

Can you see as to how to adjust the RTL side for this?  It looks like at least
some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder
if in rtl_verify_flow_info[_1] (or its callees) we can check that such
calls come
first ... they might not since IIRC we do _not_ preserve abnormal edges when
expanding RTL (there's some existing bug about this and how this breaks some
setjmp tests) (but we try to recompute them?).

Sorry about the back-and-forth again ... your original patch looks OK for the
GIMPLE side but can you amend the cfghooks.{cc,h} documentation to
summarize our findings and
the desired semantics of duplicate_block in this respect?

Thanks,
Richard.

> > > This implies patch 1/3 [1] unnecessary blocks sinking to computed goto targets.
> > > [1] https://gcc.gnu.org/pipermail/gcc-patches/2022-January/588498.html
> > >
> > > How would you like to proceed here? Is my initial patch ok?
> >
> > Hmm, so for returns twice calls duplicate_block correctly copies the call
> > and redirects the provided incoming edge to it.  The API does not
> > handle adding any further incoming edges - the caller would be responsible
> > for this.  So I still somewhat fail to see the point here.  If tracer does not
> > handle extra incoming edges properly then we need to fix tracer?
>
> I think abnormal edges corresponding to computed gotos are fine: we are
> attempting to create a chain of blocks with no incoming edges in the middle,
> right? Destinations of computed gotos remain at labels of original blocks.
>
> Agreed about correcting this in the tracer.
>
> > This also includes non-local goto (we seem to copy non-local labels just
> > fine - wasn't there a bugreport about this!?).
>
> Sorry, no idea about this.
>
> > So I think can_duplicate_block_p is the wrong place to fix (the RTL side
> > would need a similar fix anyhow?)
>
> Right. I'm happy to leave both RTL and GIMPLE can_duplicate_block_p as is,
> and instead constrain just the tracer. Alternative patch below:
>
>         * tracer.cc (analyze_bb): Disallow duplication of returns_twice calls.
>
> diff --git a/gcc/tracer.cc b/gcc/tracer.cc
> index 64517846d..422e2b6a7 100644
> --- a/gcc/tracer.cc
> +++ b/gcc/tracer.cc
> @@ -132,14 +132,19 @@ analyze_bb (basic_block bb, int *count)
>    gimple *stmt;
>    int n = 0;
>
> +  bool can_dup = can_duplicate_block_p (CONST_CAST_BB (bb));
> +
>    for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>      {
>        stmt = gsi_stmt (gsi);
>        n += estimate_num_insns (stmt, &eni_size_weights);
> +      if (can_dup && cfun->calls_setjmp && gimple_code (stmt) == GIMPLE_CALL
> +         && gimple_call_flags (stmt) & ECF_RETURNS_TWICE)
> +       can_dup = false;
>      }
>    *count = n;
>
> -  cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb)));
> +  cache_can_duplicate_bb_p (bb, can_dup);
>  }
>
>  /* Return true if E1 is more frequent than E2.  */
>
  

On Thu, 14 Jul 2022, Richard Biener wrote:

> Indeed.  Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right".
> 
> When copying a block we do not copy labels so any "jumps" remain to the original
> block and thus we are indeed able to isolate normal control flow.  Given that
> returns_twice functions _do_ seem to be special, and also special as to how
> we handle other abnormal receivers in duplicate_block.

We do? Sorry, I don't see what you mean here, can you point me to specific lines?

> So it might indeed make sense to special-case them in can_duplicate_block_p
> ... (sorry for going back-and-forth ...)
> 
> Note that I think this detail of duplicate_block (the function) and the hook
> needs to be better documented (the semantics on incoming edges, not duplicating
> labels used for incoming control flow).
> 
> Can you see as to how to adjust the RTL side for this?  It looks like at least
> some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder
> if in rtl_verify_flow_info[_1] (or its callees) we can check that such
> calls come
> first ... they might not since IIRC we do _not_ preserve abnormal edges when
> expanding RTL (there's some existing bug about this and how this breaks some
> setjmp tests) (but we try to recompute them?).

No, we emit arguments/return value handling before/after a REG_SETJMP call,
and yeah, we don't always properly recompute abnormal edges, so improving
RTL in this respect seems hopeless. For example, it is easy enough to create
a testcase where bb-reordering duplicates a returns_twice call, although it
runs so late that perhaps later passes don't care:

// gcc -O2 --param=max-grow-copy-bb-insns=100
__attribute__((returns_twice))
int rtwice(int);
int g1(int), g2(int);
void f(int i)
{
  do {
    i = i%2 ? g1(i) : g2(i);
  } while (i = rtwice(i));
}

FWIW, I also investigated https://gcc.gnu.org/PR101347

> Sorry about the back-and-forth again ... your original patch looks OK for the
> GIMPLE side but can you amend the cfghooks.{cc,h} documentation to
> summarize our findings and
> the desired semantics of duplicate_block in this respect?

Like below?

---8<---

Subject: [PATCH v3] tree-cfg: do not duplicate returns_twice calls

A returns_twice call may have associated abnormal edges that correspond
to the "second return" from the call. If the call is duplicated, the
copies of those edges also need to be abnormal, but e.g. tracer does not
enforce that. Just prohibit the (unlikely to be useful) duplication.

gcc/ChangeLog:

	* cfghooks.cc (duplicate_block): Expand comment.
	* tree-cfg.cc (gimple_can_duplicate_bb_p): Reject blocks with
	calls that may return twice.
---
 gcc/cfghooks.cc | 13 ++++++++++---
 gcc/tree-cfg.cc |  7 +++++--
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc
index e435891fa..c6ac9532c 100644
--- a/gcc/cfghooks.cc
+++ b/gcc/cfghooks.cc
@@ -1086,9 +1086,16 @@ can_duplicate_block_p (const_basic_block bb)
   return cfg_hooks->can_duplicate_block_p (bb);
 }
 
-/* Duplicates basic block BB and redirects edge E to it.  Returns the
-   new basic block.  The new basic block is placed after the basic block
-   AFTER.  */
+/* Duplicate basic block BB, place it after AFTER (if non-null) and redirect
+   edge E to it (if non-null).  Return the new basic block.
+
+   If BB contains a returns_twice call, the caller is responsible for recreating
+   incoming abnormal edges corresponding to the "second return" for the copy.
+   gimple_can_duplicate_bb_p rejects such blocks, while RTL likes to live
+   dangerously.
+
+   If BB has incoming abnormal edges for some other reason, their destinations
+   should be tied to label(s) of the original BB and not the copy.  */
 
 basic_block
 duplicate_block (basic_block bb, edge e, basic_block after, copy_bb_data *id)
diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
index f846dc2d8..5bcf78198 100644
--- a/gcc/tree-cfg.cc
+++ b/gcc/tree-cfg.cc
@@ -6346,12 +6346,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
     {
       gimple *g = gsi_stmt (gsi);
 
-      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
+      /* Prohibit duplication of returns_twice calls, otherwise associated
+	 abnormal edges also need to be duplicated properly.
+	 An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
 	 duplicated as part of its group, or not at all.
 	 The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
 	 group, so the same holds there.  */
       if (is_gimple_call (g)
-	  && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
+	  && (gimple_call_flags (g) & ECF_RETURNS_TWICE
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
 	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
  

On Thu, Jul 14, 2022 at 10:12 PM Alexander Monakov <amonakov@ispras.ru> wrote:
>
>
> On Thu, 14 Jul 2022, Richard Biener wrote:
>
> > Indeed.  Guess that's what __builtin_setjmp[_receiver] for SJLJ_EH got "right".
> >
> > When copying a block we do not copy labels so any "jumps" remain to the original
> > block and thus we are indeed able to isolate normal control flow.  Given that
> > returns_twice functions _do_ seem to be special, and also special as to how
> > we handle other abnormal receivers in duplicate_block.
>
> We do? Sorry, I don't see what you mean here, can you point me to specific lines?

I'm referring to

      stmt = gsi_stmt (gsi);
      if (gimple_code (stmt) == GIMPLE_LABEL)
        continue;

but indeed, looking again we do _not_ skip a __builtin_setjmp_receiver
(but I don't
exactly remember the CFG setup with SJLJ EH and setjmp_{receiver,setup}.

> > So it might indeed make sense to special-case them in can_duplicate_block_p
> > ... (sorry for going back-and-forth ...)
> >
> > Note that I think this detail of duplicate_block (the function) and the hook
> > needs to be better documented (the semantics on incoming edges, not duplicating
> > labels used for incoming control flow).
> >
> > Can you see as to how to adjust the RTL side for this?  It looks like at least
> > some places set a REG_SETJMP note on call_insns (emit_call_1), I wonder
> > if in rtl_verify_flow_info[_1] (or its callees) we can check that such
> > calls come
> > first ... they might not since IIRC we do _not_ preserve abnormal edges when
> > expanding RTL (there's some existing bug about this and how this breaks some
> > setjmp tests) (but we try to recompute them?).
>
> No, we emit arguments/return value handling before/after a REG_SETJMP call,
> and yeah, we don't always properly recompute abnormal edges, so improving
> RTL in this respect seems hopeless.

:/  (but yes, nobody got to fixing PR57067 in the last 10 years)

> For example, it is easy enough to create
> a testcase where bb-reordering duplicates a returns_twice call, although it
> runs so late that perhaps later passes don't care:
>
> // gcc -O2 --param=max-grow-copy-bb-insns=100
> __attribute__((returns_twice))
> int rtwice(int);
> int g1(int), g2(int);
> void f(int i)
> {
>   do {
>     i = i%2 ? g1(i) : g2(i);
>   } while (i = rtwice(i));
> }
>
> FWIW, I also investigated https://gcc.gnu.org/PR101347
>
> > Sorry about the back-and-forth again ... your original patch looks OK for the
> > GIMPLE side but can you amend the cfghooks.{cc,h} documentation to
> > summarize our findings and
> > the desired semantics of duplicate_block in this respect?
>
> Like below?

Yes.

Thanks and sorry for the back and forth - this _is_ a mightly
complicated area ...

Richard.

> ---8<---
>
> Subject: [PATCH v3] tree-cfg: do not duplicate returns_twice calls
>
> A returns_twice call may have associated abnormal edges that correspond
> to the "second return" from the call. If the call is duplicated, the
> copies of those edges also need to be abnormal, but e.g. tracer does not
> enforce that. Just prohibit the (unlikely to be useful) duplication.
>
> gcc/ChangeLog:
>
>         * cfghooks.cc (duplicate_block): Expand comment.
>         * tree-cfg.cc (gimple_can_duplicate_bb_p): Reject blocks with
>         calls that may return twice.
> ---
>  gcc/cfghooks.cc | 13 ++++++++++---
>  gcc/tree-cfg.cc |  7 +++++--
>  2 files changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/gcc/cfghooks.cc b/gcc/cfghooks.cc
> index e435891fa..c6ac9532c 100644
> --- a/gcc/cfghooks.cc
> +++ b/gcc/cfghooks.cc
> @@ -1086,9 +1086,16 @@ can_duplicate_block_p (const_basic_block bb)
>    return cfg_hooks->can_duplicate_block_p (bb);
>  }
>
> -/* Duplicates basic block BB and redirects edge E to it.  Returns the
> -   new basic block.  The new basic block is placed after the basic block
> -   AFTER.  */
> +/* Duplicate basic block BB, place it after AFTER (if non-null) and redirect
> +   edge E to it (if non-null).  Return the new basic block.
> +
> +   If BB contains a returns_twice call, the caller is responsible for recreating
> +   incoming abnormal edges corresponding to the "second return" for the copy.
> +   gimple_can_duplicate_bb_p rejects such blocks, while RTL likes to live
> +   dangerously.
> +
> +   If BB has incoming abnormal edges for some other reason, their destinations
> +   should be tied to label(s) of the original BB and not the copy.  */
>
>  basic_block
>  duplicate_block (basic_block bb, edge e, basic_block after, copy_bb_data *id)
> diff --git a/gcc/tree-cfg.cc b/gcc/tree-cfg.cc
> index f846dc2d8..5bcf78198 100644
> --- a/gcc/tree-cfg.cc
> +++ b/gcc/tree-cfg.cc
> @@ -6346,12 +6346,15 @@ gimple_can_duplicate_bb_p (const_basic_block bb)
>      {
>        gimple *g = gsi_stmt (gsi);
>
> -      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
> +      /* Prohibit duplication of returns_twice calls, otherwise associated
> +        abnormal edges also need to be duplicated properly.
> +        An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
>          duplicated as part of its group, or not at all.
>          The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
>          group, so the same holds there.  */
>        if (is_gimple_call (g)
> -         && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
> +         && (gimple_call_flags (g) & ECF_RETURNS_TWICE
> +             || gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
>               || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
> --
> 2.35.1
>
  

On Tue, 19 Jul 2022, Richard Biener wrote:

> > Like below?
> 
> Yes.
> 
> Thanks and sorry for the back and forth - this _is_ a mightly
> complicated area ...

No problem!  This is the good, healthy kind of back-and-forth, and
I am grateful.

Pushed, including the tree-cfg validator enhancement in patch 3/3.

Alexander