[Bug debug/108656] New: [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0

[Bug debug/108656] New: [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0

[asolokha at gmx dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

            Bug ID: 108656
           Summary: [12/13 Regression] '-fcompare-debug' failure (length)
                    w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param
                    early-inlining-insns=0
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Keywords: compare-debug-failure
          Severity: normal
          Priority: P3
         Component: debug
          Assignee: unassigned at gcc dot gnu.org
          Reporter: asolokha at gmx dot com
  Target Milestone: ---

g++ 13.0.0 20220515 snapshot (g:18547874ee205d830acb31f1e3c1c89fc7725c14) fails
-fcompare-debug check when compiling the following testcase w/ -O2
-fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0:

volatile int q;
int m, n;

int
quux (int x)
{
  return x;
}

int
corge (int x, int y)
{
  if (x < 0 || x > y)
    return 1;

 return 0;
}

int
baz (void)
{
  return m ? q : 0;
}

__attribute__ ((pure, returns_twice)) int
bar (void)
{
  (void) baz ();

  return 0;
}

int
foo (void)
{
  int a = quux (0);
  int b = corge (bar (), n);

  return 0;
}

 % gcc-13 -O2 -fcompare-debug -fno-ipa-pure-const -fno-tree-dce --param
early-inlining-insns=0 -c c46sgk3m.c
gcc-13: error: c46sgk3m.c: '-fcompare-debug' failure (length)

gkd diff:

 (note # 0 0 NOTE_INSN_DELETED)

 Declarations used by foo, sorted by DECL_UID:
-1:   static volatile int q;
-0:   static int m;
+2:   static volatile int q;
+1:   static int m;
+0:   static int n;

 ;; Function foo (foo, funcdef_no=4, cgraph_uid=5, symbol_order=7)

@@ -184,17 +185,17 @@
 (jump_insn # 0 0 2 (set (pc)
         (if_then_else (eq (reg:CCZ 17 flags)
                 (const_int 0 [0]))
-            (label_ref #)
+            (label_ref:DI #)
             (pc))) "c46sgk3m.c":22:16# {*jcc}
      (expr_list:REG_DEAD (reg:CCZ 17 flags)
         (int_list:REG_BR_PROB 536870916 (nil)))
- -> 12)
+ -> 13)
 (note # 0 0 [bb 3] NOTE_INSN_BASIC_BLOCK)
-(insn:TI # 0 0 3 (set (reg:SI 0 ax [orig:83 iftmp.0_10 ] [83])
+(insn:TI # 0 0 3 (set (reg:SI 0 ax [orig:84 iftmp.0_10 ] [84])
         (mem/v/c:SI (symbol_ref:DI ("q") [flags 0x2]  <var_decl # q>) [ q+0 S4
A32])) "c46sgk3m.c":22:16 discrim 1# {*movsi_internal}
-     (expr_list:REG_UNUSED (reg:SI 0 ax [orig:83 iftmp.0_10 ] [83])
+     (expr_list:REG_UNUSED (reg:SI 0 ax [orig:84 iftmp.0_10 ] [84])
         (nil)))
-(code_label # 0 0 4 12 (nil) [1 uses])
+(code_label # 0 0 4 13 (nil) [1 uses])
 (note # 0 0 [bb 4] NOTE_INSN_BASIC_BLOCK)
 (insn:TI # 0 0 4 (parallel [
             (set (reg:DI 0 ax)

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0

[asolokha at gmx dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #1 from Arseny Solokha <asolokha at gmx dot com> ---
The snapshot date is of course wrong. It should read "gcc 13.0.1 20230119
snapshot (g:2e32a12c04c72f692a7bd119fd3e4e5b74392c9d)".

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P2
   Last reconfirmed|                            |2023-02-03
                 CC|                            |jakub at gcc dot gnu.org
            Summary|[12/13 Regression]          |[12/13 Regression]
                   |'-fcompare-debug' failure   |'-fcompare-debug' failure
                   |(length) w/ -O2             |(length) w/ -O2
                   |-fno-ipa-pure-const         |-fno-ipa-pure-const
                   |-fno-tree-dce --param       |-fno-tree-dce --param
                   |early-inlining-insns=0      |early-inlining-insns=0
                   |                            |since r12-5236
   Target Milestone|---                         |12.3
             Status|UNCONFIRMED                 |NEW
     Ever confirmed|0                           |1

--- Comment #2 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r12-5236-g5aa91072e24c1e16a5e .

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hubicka at gcc dot gnu.org,
                   |                            |rguenth at gcc dot gnu.org

--- Comment #3 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
I think the problem is in the bogus combination of attributes, pure (or const)
and returns_twice. For returns_twice we force the call to be the first
statement in a bb (stmt_starts_bb_p returns true for it) with an AB edge to it
and expect it to be the last one as well with an AB edge out of it.  For normal
returns_twice calls that is the case, the fact that there is returns_twice
results in cfun->calls_setjmp and in that case call_can_make_abnormal_goto will
be true (so among others the call is marked as ctrl altering and eventually
stmt_ends_bb_p).
But if returns_twice is mixed with const, pure or leaf attributes, this all
falls apart.
The exact reason for the -fcompare-debug failure is that we have the weird
;;   basic block 4, loop depth 0, maybe hot
;;    prev block 3, next block 5, flags: (NEW)
;;    pred:       3 (FALLTHRU)
;;                5 (ABNORMAL,DFS_BACK)
  # _5(ab) = PHI <_12(3), _6(ab)(5)>
  _2 = bar ();
  _14 = corge (_2, _5(ab));
  goto <bb 6>; [INV]
;;    succ:       6 (FALLTHRU)
;;                5 (ABNORMAL)
pre-einline (note, corge isn't pure/const/leaf and so unlike bar is assumed to
maybe doing abnormal goto), einline turns it into:
;;   basic block 3, loop depth 0, maybe hot
;;    prev block 2, next block 4, flags: (NEW, REACHABLE)
;;    pred:       2 (FALLTHRU)
;;                4 (ABNORMAL,DFS_BACK)
  # _5(ab) = PHI <_12(2), _6(ab)(4)>
  # DEBUG BEGIN_STMT
  baz ();
  # DEBUG BEGIN_STMT
  _19 = 0;
  _2 = _19;
  _14 = corge (_2, _5(ab));
  goto <bb 5>; [INV]
;;    succ:       5 (FALLTHRU)
;;                4 (ABNORMAL)
(with the debug stmts missing for -g0), no returns_twice nor anything else
actually needing abnormal stuff in the cfg, but bar wasn't the last stmt in bb,
so no purging was done.  Later on, expand_calL_inline is called on the bb which
has just
;;   basic block 3, loop depth 0, maybe hot
;;    prev block 2, next block 4, flags: (NEW, REACHABLE)
;;    pred:       2 (FALLTHRU)
;;                4 (ABNORMAL,DFS_BACK)
  # _5(ab) = PHI <_12(2), _6(ab)(4)>
  # DEBUG BEGIN_STMT
  baz ();
  # DEBUG BEGIN_STMT
  goto <bb 5>; [INV]
;;    succ:       5 (FALLTHRU)
;;                4 (ABNORMAL)
and return_block is after splitting in one case just baz (); call and in
another case
baz () call + debug stmt.
  /* If the GIMPLE_CALL was in the last statement of BB, it may have
     been the source of abnormal edges.  In this case, schedule
     the removal of dead abnormal edges.  */
  gsi = gsi_start_bb (return_block);
  gsi_next (&gsi);
  purge_dead_abnormal_edges = gsi_end_p (gsi);
So, purge_dead_abnormal_edges is set for -g0 but not -g and as after inlining
baz there is nothing that would need an abnormal edge, we drop
.ABNORMAL_DISPATCHER in the former case and not the latter.

I wonder if we shouldn't reject mixing const/pure/leaf attributes with
returns_twice, like we already warn and ignore returns_twice attribute when
mixed with noreturn.

Or, I think we'd need to make sure returns_twice results in
call_can_make_abnormal_goto
true, and perhaps stmt_can_terminate_bb_p true too.  Though, still I wonder
what will happen if we DCE the returns_twice calls, wonder if we at least drop
the abnormal successor edges then.

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #4 from Richard Biener <rguenth at gcc dot gnu.org> ---
In general we avoid disallowing attribute combinations that at least in theory
make sense.  pure/const are about memory side-effects while returns_twice is
about control flow, so in this regard I don't see how they should conflict.

That we exclude !gimple_has_side_effects from call_can_make_abnormal_goto
is somewhat of a chicken-and-egg thing - "side effect" is something not
explicitely encoded in the IL but a returns_twice results in explicit
encoding via abnormal edges.  But then we cannot use that for CFG
construction.

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #5 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Created attachment 54412
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=54412&action=edit
gcc13-pr108656.patch

So shall we fix it like this then?

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #6 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #5)
> Created attachment 54412 [details]
> gcc13-pr108656.patch
> 
> So shall we fix it like this then?

But isn't this the wrong "side"?  returns_twice means it is the abnormal
control _receiver_, it doesn't perform an abnormal goto itself.

stmt_starts_bb_p is correct here, so where does it go wrong?

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to Richard Biener from comment #6)
> (In reply to Jakub Jelinek from comment #5)
> > Created attachment 54412 [details]
> > gcc13-pr108656.patch
> > 
> > So shall we fix it like this then?
> 
> But isn't this the wrong "side"?  returns_twice means it is the abnormal
> control _receiver_, it doesn't perform an abnormal goto itself.
> 
> stmt_starts_bb_p is correct here, so where does it go wrong?

I think it needs both.  The thing is, when it returns the second time, it does
that again by returning from itself, not through returning from some unrelated
function.
Say, if we have pure + returns_twice call and no other call in a function, the
abnormal edge to the pure + returns_twice call would be optimized away, even
when the call remains, because there would be no edge from some call to the
.ABNORMAL_DISPATCHER block.
__attribute__((pure, returns_twice)) int foo (void);

int
bar (void)
{
  for (int i = 0; i < 64; ++i)
    {
      int x = foo ();
      if (x == 26)
        return -42;
    }
  return 42;
}
doesn't even have because of this any abnormal edges created.
Or, if there is some other non-pure call somewhere else, we model through the
abnormal
edges that that other call can pass control back to the start of the
returns_twice call to make it return again.

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #8 from Richard Biener <rguenth at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #7)
> (In reply to Richard Biener from comment #6)
> > (In reply to Jakub Jelinek from comment #5)
> > > Created attachment 54412 [details]
> > > gcc13-pr108656.patch
> > > 
> > > So shall we fix it like this then?
> > 
> > But isn't this the wrong "side"?  returns_twice means it is the abnormal
> > control _receiver_, it doesn't perform an abnormal goto itself.
> > 
> > stmt_starts_bb_p is correct here, so where does it go wrong?
> 
> I think it needs both.  The thing is, when it returns the second time, it
> does that again by returning from itself, not through returning from some
> unrelated function.
> Say, if we have pure + returns_twice call and no other call in a function,
> the abnormal edge to the pure + returns_twice call would be optimized away,
> even when the call remains, because there would be no edge from some call to
> the .ABNORMAL_DISPATCHER block.
> __attribute__((pure, returns_twice)) int foo (void);
> 
> int
> bar (void)
> {
>   for (int i = 0; i < 64; ++i)
>     {
>       int x = foo ();
>       if (x == 26)
>         return -42;
>     }
>   return 42;
> }
> doesn't even have because of this any abnormal edges created.
> Or, if there is some other non-pure call somewhere else, we model through
> the abnormal
> edges that that other call can pass control back to the start of the
> returns_twice call to make it return again.

Ah, but then the issue is that we assume that 'foo' doesn't longjmp,
independent on whether it is returns_twice or not?  Can a setjmp ()
function perform a longjmp () to its own context?  Would it even appear
as returning twice then?  Would calling setjmp in a loop like above
and then jumping to another iteration via longjmp even be valid?

That said, for your example I would think not having any abnormal edges
is correct - there's no frame that could transfer control back to the
returns_twice receiver, no?

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #9 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
In the setjmp/longjmp case obviously you need some other function that will do
the longjmp, on the other side setjmp as pure makes no sense because setjmp has
to remember pc/sp etc. in some jump buffer, so it isn't pure.  Ditto getcontext
etc.
But returns_twice isn't just setjmp, it is also fork/vfork or anything else
that can arrange multiple returns through other means.

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Anyway, if we decided that it is ok to have just the incoming ab edges, we'd
need to change any code that can DCE or inline calls to update abnormal edges
not just for the case where the last stmt used to be can_make_abnormal_goto and
no longer is, but also for the case when the first stmt was returns_twice call
and no longer is, in that case we'd need to drop that abnormal edge pointing to
the start of the bb from the .ABNORMAL_DISPATCHER block.

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #11 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 6 Feb 2023, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656
> 
> --- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> Anyway, if we decided that it is ok to have just the incoming ab edges, we'd
> need to change any code that can DCE or inline calls to update abnormal edges
> not just for the case where the last stmt used to be can_make_abnormal_goto and
> no longer is, but also for the case when the first stmt was returns_twice call
> and no longer is, in that case we'd need to drop that abnormal edge pointing to
> the start of the bb from the .ABNORMAL_DISPATCHER block.

Doesn't cleanup CFG do this via removal of the unreachable 
.ABNORMAL_DISPATCHER?

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[jakub at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
(In reply to rguenther@suse.de from comment #11)
> On Mon, 6 Feb 2023, jakub at gcc dot gnu.org wrote:
> 
> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656
> > 
> > --- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> > Anyway, if we decided that it is ok to have just the incoming ab edges, we'd
> > need to change any code that can DCE or inline calls to update abnormal edges
> > not just for the case where the last stmt used to be can_make_abnormal_goto and
> > no longer is, but also for the case when the first stmt was returns_twice call
> > and no longer is, in that case we'd need to drop that abnormal edge pointing to
> > the start of the bb from the .ABNORMAL_DISPATCHER block.
> 
> Doesn't cleanup CFG do this via removal of the unreachable 
> .ABNORMAL_DISPATCHER?

If all non-pure calls are removed (all can_make_abnormal_goto to be precise),
sure.  Or if we reset and recompute cfun->calls_setjmp and then do cfg cleanup.
The problem on the above testcase is that it can never happen or takes many
passes.
The returns_twice function is gone during einline pass, but there are other
calls that can make abnormal goto (well, have side-effects and nothing noticed
we don't have any returns_twice calls anymore).  So we have an ab edge pointing
to a bb that doesn't start with returns_twice function, that alone looks wrong
to me.  During IPA we then inline further functions and diverge on when we
trigger the removal on outgoing abnormal edges because of debug stmts.

[Bug debug/108656] [12/13 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[rguenther at suse dot de]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

--- Comment #13 from rguenther at suse dot de <rguenther at suse dot de> ---
On Mon, 6 Feb 2023, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656
> 
> --- Comment #12 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> (In reply to rguenther@suse.de from comment #11)
> > On Mon, 6 Feb 2023, jakub at gcc dot gnu.org wrote:
> > 
> > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656
> > > 
> > > --- Comment #10 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> > > Anyway, if we decided that it is ok to have just the incoming ab edges, we'd
> > > need to change any code that can DCE or inline calls to update abnormal edges
> > > not just for the case where the last stmt used to be can_make_abnormal_goto and
> > > no longer is, but also for the case when the first stmt was returns_twice call
> > > and no longer is, in that case we'd need to drop that abnormal edge pointing to
> > > the start of the bb from the .ABNORMAL_DISPATCHER block.
> > 
> > Doesn't cleanup CFG do this via removal of the unreachable 
> > .ABNORMAL_DISPATCHER?
> 
> If all non-pure calls are removed (all can_make_abnormal_goto to be precise),
> sure.  Or if we reset and recompute cfun->calls_setjmp and then do cfg cleanup.
> The problem on the above testcase is that it can never happen or takes many
> passes.
> The returns_twice function is gone during einline pass, but there are other
> calls that can make abnormal goto (well, have side-effects and nothing noticed
> we don't have any returns_twice calls anymore).  So we have an ab edge pointing
> to a bb that doesn't start with returns_twice function, that alone looks wrong
> to me.  During IPA we then inline further functions and diverge on when we
> trigger the removal on outgoing abnormal edges because of debug stmts.

I've fixed a lot of issues like this, even implemented a verifier, but 
that's still too trigger happy.  So yes, there are some issues left
and the only hard assert I've put in is the DCE one that makes sure
abnormal dispatchers are not discovered "late".

[Bug debug/108656] [12/13/14 Regression] '-fcompare-debug' failure (length) w/ -O2 -fno-ipa-pure-const -fno-tree-dce --param early-inlining-insns=0 since r12-5236

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108656

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|12.3                        |12.4

--- Comment #14 from Richard Biener <rguenth at gcc dot gnu.org> ---
GCC 12.3 is being released, retargeting bugs to GCC 12.4.