[Bug tree-optimization/98834] New: Code path incorrectly determined to be unreachable

[Bug tree-optimization/98834] New: Code path incorrectly determined to be unreachable

[kretz at kde dot org]

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

            Bug ID: 98834
           Summary: Code path incorrectly determined to be unreachable
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: tree-optimization
          Assignee: unassigned at gcc dot gnu.org
          Reporter: kretz at kde dot org
  Target Milestone: ---
            Target: x86_64-pc-linux-gnu

Created attachment 50054
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50054&action=edit
test case

The attached testcase compiles to a main function without return statement, as
if the compiler determines the code path is unreachable.

Compile with -O2. If you compile with -O2 -fno-tree-vrp, the return statement
is emitted.

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Last reconfirmed|                            |2021-01-26
           Keywords|                            |needs-bisection, wrong-code
                 CC|                            |jamborm at gcc dot gnu.org,
                   |                            |marxin at gcc dot gnu.org
     Ever confirmed|0                           |1
          Component|tree-optimization           |ipa
   Target Milestone|---                         |10.3
           Priority|P3                          |P2
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Richard Biener <rguenth at gcc dot gnu.org> ---
Confirmed, the __builtin_unreachable is introduced by inlining:

Considering _S_multiplies.isra/23 with 12 size
 to be inlined into int main()/13 in t.C:54
 Estimated badness is -inf, frequency 1.00.
    Badness calculation for int main()/13 -> _S_multiplies.isra/23
      size growth -2, time 2.000000 unspec 10.000000 IPA hints: declared_inline
big_speedup
      -inf: Growth -2 <= 0
      Adjusted by hints -inf
                Accounting size:2.00, time:2.00 on predicate exec:(true)
Introduced new external node (void __builtin_unreachable()/24).
Processing frequency _S_multiplies.isra/23
  Called by int main()/13 that is executed once
                Accounting size:-4.00, time:-13.00 on predicate exec:(true)
t.C:54:62: optimized:  Inlined _S_multiplies.isra/23 into int main()/13 which
now has time 8.000000 and size 9, net change of -14.

which points to IPA SRA.  VRP then simplifies __builtin_constant_p seen as

  _12 = __builtin_constant_p (__xx_11);
  if (_12 != 0)
    goto <bb 4>; [100.00%]
  else
    goto <bb 3>; [0.00%]

  <bb 3> [count: 0]:
  __builtin_unreachable ();

  <bb 4> [local count: 1073741824]:
  MEM <int> [(struct _Tuple *)&D.2557] = 0;

and uses conservatively false but that runs into __builtin_unreachable () ...

The IPA inline transform ends us with

  _12 = __builtin_constant_p (__xx_11);
  if (_12 != 0)
    goto <bb 4>; [100.00%]
  else
    goto <bb 3>; [0.00%]

  <bb 3> [count: 0]:
  _13 = _16(D);
  __builtin_unreachable ();

  <bb 4> [local count: 1073741824]:
  # SR.30_15 = PHI <__y_10(2)>

which eventually shows the reason (the unused argument?)

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[kretz at kde dot org]

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

--- Comment #2 from Matthias Kretz (Vir) <kretz at kde dot org> ---
This is reduced from a larger (4MB) testcase which doesn't have any unused
arguments.

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[kretz at kde dot org]

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

--- Comment #3 from Matthias Kretz (Vir) <kretz at kde dot org> ---
Created attachment 50055
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=50055&action=edit
unreduced test case

This is the test case I gave to C-Vise. It's already reduced from a more
confusing test, where preceding COMPARE(...) would fail by taking the fail path
unconditionally. Which now makes sense, since GCC considered the following code
to be unreachable.

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[jakub at gcc dot gnu.org]

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

--- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
Started with r10-3106-g46dfa8ad6c18feb45d35734eae38798edb7c38cd
Anyway, I wonder if this isn't similar to the cases where the inliner
optimistically assumed that __builtin_constant_p will fold to true but didn't
actually fold it that way, and then later on that didn't happen?

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[rguenther at suse dot de]

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

--- Comment #5 from rguenther at suse dot de <rguenther at suse dot de> ---
On Tue, 26 Jan 2021, jakub at gcc dot gnu.org wrote:

> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98834
> 
> --- Comment #4 from Jakub Jelinek <jakub at gcc dot gnu.org> ---
> Started with r10-3106-g46dfa8ad6c18feb45d35734eae38798edb7c38cd
> Anyway, I wonder if this isn't similar to the cases where the inliner
> optimistically assumed that __builtin_constant_p will fold to true but didn't
> actually fold it that way, and then later on that didn't happen?

Yes, which is why I made it IPA and CCed Martin ...

IMHO the __builtin_unreachable () introduced is bogus.

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[jamborm at gcc dot gnu.org]

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

Martin Jambor <jamborm at gcc dot gnu.org> changed:

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

--- Comment #6 from Martin Jambor <jamborm at gcc dot gnu.org> ---
(In reply to Jakub Jelinek from comment #4)
> Started with r10-3106-g46dfa8ad6c18feb45d35734eae38798edb7c38cd
> Anyway, I wonder if this isn't similar to the cases where the inliner
> optimistically assumed that __builtin_constant_p will fold to true but
> didn't actually fold it that way, and then later on that didn't happen?

It is exactly that.  We have one __builtin_constant_p that is called
if another, previous __builtin_constant_p returns false.  Inlining
assumes the previous one will be folded to true because it knows it is
called on a constant, albeit one read from an aggregate, and therefore
assumes the latter will never happen.  But after inlining, the
situation looks like this (some statements omitted):

  <bb 2> [local count: 1073741824]:
  MEM <intD.9> [(struct simdD.2582 *)&__xD.2752] = 0;
  MEM <intD.9> [(struct simdD.2582 *)&__xD.2752 + 4B] = 0;
  ...
  __xD.2820 = __xD.2752._M_dataD.2591;
  ...
  __xx_10 = MEM <intD.9> [(struct _TupleD.2456 *)&__xD.2820];
  _11 = __builtin_constant_p (__xx_10);

And we need a SRA + CCP to make the constant travel from the first
assignment to the __builtin_constant_p (__xx_10).  And SRA does take
place only after VRP which already decides to fold
__builtin_constant_p to false.

A quick and dirty (and potentially regression causing) way to fix that
is to declare __builtin_constant_p not working on aggregate values:

diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
index 18bbae145b9..c319323b31e 100644
--- a/gcc/ipa-fnsummary.c
+++ b/gcc/ipa-fnsummary.c
@@ -1638,7 +1638,8 @@ set_cond_stmt_execution_predicate (struct
ipa_func_body_info *fbi,
       || gimple_call_num_args (set_stmt) != 1)
     return;
   op2 = gimple_call_arg (set_stmt, 0);
-  if (!decompose_param_expr (fbi, set_stmt, op2, &index, &param_type,
&aggpos))
+  if (!decompose_param_expr (fbi, set_stmt, op2, &index, &param_type, &aggpos)
+      || aggpos.agg_contents)
     return;
   if (!aggpos.by_ref)
     add_builtin_constant_p_parm (summary, index);

(the last condition would then also need to be turned into a comment
...and possibly an assert).  But it feels like too big a hammer.

Alternatively, we could just resolve the builtin at IPA time.  We'd
need to store the predicate derived from its argument to
ipa_call_summary and then at IPA time redirect it to some
__builtin_true_p when IPA figures out it has to be true, which we
would then redirect in cgraph_edge::redirect_call_stmt_to_callee.
Honza, would that be a good idea?

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[rguenth at gcc dot gnu.org]

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

--- Comment #7 from Richard Biener <rguenth at gcc dot gnu.org> ---
So for the missed optimization we run into

  /* 5) For aggregate copies translate the reference through them if
     the copy kills ref.  */
  else if (data->vn_walk_kind == VN_WALKREWRITE
...
      /* Adjust *ref from the new operands.  */
      ao_ref rhs1_ref;
      ao_ref_init (&rhs1_ref, rhs1);
      if (!ao_ref_init_from_vn_reference (&r, ao_ref_alias_set (&rhs1_ref),
                                          ao_ref_base_alias_set (&rhs1_ref),
                                          vr->type, vr->operands))
        return (void *)-1;
      /* This can happen with bitfields.  */
      if (maybe_ne (ref->size, r.size))
        return (void *)-1;

because the IL looks like

  __xD.2835 = __xD.2753._M_dataD.2625;
  __xx_11 = MEM <intD.9> [(struct _TupleD.2456 *)&__xD.2835];

and we try to express the load in terms of the RHS of the aggregate copy
but we end up with __xD.2753._M_dataD.2625 itself (there's no subsetting
component ref on the original load) but that loads 64 bytes, not 32 as
requested.  The code tries to handle variable index accesses and thus
doesn't simply try to compute base + offset and a corresponding MEM_REF
to look up.

The following seems to work but is otherwise untested:

diff --git a/gcc/tree-ssa-sccvn.c b/gcc/tree-ssa-sccvn.c
index e3806e55457..c47bd19a1fa 100644
--- a/gcc/tree-ssa-sccvn.c
+++ b/gcc/tree-ssa-sccvn.c
@@ -3306,7 +3306,17 @@ vn_reference_lookup_3 (ao_ref *ref, tree vuse, void
*data_,
        return (void *)-1;
       /* This can happen with bitfields.  */
       if (maybe_ne (ref->size, r.size))
-       return (void *)-1;
+       {
+         /* If the access lacks some subsetting simply apply that by
+            shortening it.  That in the end can only be successful
+            if we can pun the lookup result which in turn requires
+            exact offsets.  */
+         if (known_eq (r.size, r.max_size)
+             && known_lt (ref->size, r.size))
+           r.size = r.max_size = ref->size;
+         else
+           return (void *)-1;
+       }
       *ref = r;

       /* Do not update last seen VUSE after translating.  */

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Biener <rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:99415d0f18716deeaa8d80e929b1337968cdfa23

commit r11-7671-g99415d0f18716deeaa8d80e929b1337968cdfa23
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Mar 15 13:44:07 2021 +0100

    tree-optimization/98834 - fix optimization regression with _b_c_p

    The following makes FRE optimize a load we formerly required
    SRA + CCP for which now run after we get rid of all __builtin_constant_p
    calls.

    2021-03-15  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/98834
            * tree-ssa-sccvn.c (vn_reference_lookup_3): Handle missing
            subsetting by truncating the access size.

            * g++.dg/opt/pr98834.C: New testcase.

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[rguenth at gcc dot gnu.org]

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

--- Comment #9 from Richard Biener <rguenth at gcc dot gnu.org> ---
This patch restores the optimization of _b_c_p on trunk (and hides the issue
again).

[Bug ipa/98834] [10/11 Regression] Code path incorrectly determined to be unreachable

[marxin at gcc dot gnu.org]

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

--- Comment #10 from Martin Liška <marxin at gcc dot gnu.org> ---
@Richi: Do you still need a bisection or is what Jakub provided accurately
identified?

[Bug ipa/98834] [10 Regression] Code path incorrectly determined to be unreachable

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |rguenth at gcc dot gnu.org

--- Comment #11 from Richard Biener <rguenth at gcc dot gnu.org> ---
Mine for a backport.

[Bug ipa/98834] [10 Regression] Code path incorrectly determined to be unreachable

[cvs-commit at gcc dot gnu.org]

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

--- Comment #12 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Biener
<rguenth@gcc.gnu.org>:

https://gcc.gnu.org/g:592388d4f6e8a6adb470428fef6195694f4a3dce

commit r10-9527-g592388d4f6e8a6adb470428fef6195694f4a3dce
Author: Richard Biener <rguenther@suse.de>
Date:   Mon Mar 15 13:44:07 2021 +0100

    tree-optimization/98834 - fix optimization regression with _b_c_p

    The following makes FRE optimize a load we formerly required
    SRA + CCP for which now run after we get rid of all __builtin_constant_p
    calls.

    2021-03-15  Richard Biener  <rguenther@suse.de>

            PR tree-optimization/98834
            * tree-ssa-sccvn.c (vn_reference_lookup_3): Handle missing
            subsetting by truncating the access size.

            * g++.dg/opt/pr98834.C: New testcase.

    (cherry picked from commit 99415d0f18716deeaa8d80e929b1337968cdfa23)

[Bug ipa/98834] [10 Regression] Code path incorrectly determined to be unreachable

[rguenth at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|ASSIGNED                    |RESOLVED

--- Comment #13 from Richard Biener <rguenth at gcc dot gnu.org> ---
Fixed.