Bug 55683 - [4.8 Regression] ICE in inline_call, at ipa-inline-transform.c:270
Summary: [4.8 Regression] ICE in inline_call, at ipa-inline-transform.c:270
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.8.0
: P3 normal
Target Milestone: 4.8.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code
: 55711 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-12-14 08:56 UTC by Richard Biener
Modified: 2013-01-10 16:58 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2012-12-14 00:00:00


Attachments
preprocessed source (147.63 KB, application/octet-stream)
2012-12-14 08:56 UTC, Richard Biener
Details
proposed patch (319 bytes, patch)
2012-12-18 17:15 UTC, Jan Hubicka
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Richard Biener 2012-12-14 08:56:37 UTC
Created attachment 28949 [details]
preprocessed source

/usr/lib/gcc/i586-suse-linux/4.8/cc1plus -fpreprocessed gHud.ii -quiet -dumpbase gHud.cpp -mtune=generic -march=i586 -auxbase-strip libtron_a-gHud.o -g -O2 -Wall -Wno-long-long -version -fomit-frame-pointer -fmessage-length=0 -fstack-protector -funwind-tables -fasynchronous-unwind-tables -o gHud.s
GNU C++ (SUSE Linux) version 4.8.0 20121212 [trunk revision 194442] (i586-suse-linux)
        compiled by GNU C version 4.8.0 20121212 [trunk revision 194442], GMP version 5.0.5, MPFR version 3.1.1, MPC version 1.0
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
GNU C++ (SUSE Linux) version 4.8.0 20121212 [trunk revision 194442] (i586-suse-linux)
        compiled by GNU C version 4.8.0 20121212 [trunk revision 194442], GMP version 5.0.5, MPFR version 3.1.1, MPC version 1.0
GGC heuristics: --param ggc-min-expand=30 --param ggc-min-heapsize=4096
Compiler executable checksum: 7994fbb6f783568f8bffde327e2e9c27
tron/gHud.cpp:626:50: internal compiler error: in inline_call, at ipa-inline-transform.c:270
 static rPerFrameTask dfps(&display_hud_subby_all);
                                                  ^
libbacktrace could not find executable to open
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://bugs.opensuse.org/> for instructions.
Comment 1 Richard Biener 2012-12-14 08:58:45 UTC
Happens in quite a lot of packages.
Comment 2 Jakub Jelinek 2012-12-14 09:57:35 UTC
Reduced testcase for -O2 (both x86_64-linux and i686-linux hit this):

double foo ();
struct B
{
  bool b1 () { return b3 (); }
  void b2 ();
  virtual bool b3 ();
};
struct C
{
  C () {}
  bool
  c1 (float x, float y)
  {
    if (x != c3 || y != c4)
      c2.b2 ();
    return c2.b1 ();
  }
  B c2;
  float c3, c4;
};

void
bar ()
{
  static C c;
  c.c1 (60, (int) foo ());
}
Comment 3 Richard Biener 2012-12-14 10:25:53 UTC
263     #ifdef ENABLE_CHECKING
264       /* Verify that estimated growth match real growth.  Allow off-by-one
265          error due to INLINE_SIZE_SCALE roudoff errors.  */
266       gcc_assert (!update_overall_summary || !overall_size
267                   || abs (estimated_growth - (new_size - old_size)) <= 1
268                   /* FIXME: a hack.  Edges with false predicate are accounted
269                      wrong, we should remove them from callgraph.  */
270                   || predicated);
(gdb) p update_overall_summary
$1 = true
(gdb) p overall_size
$2 = (int *) 0x1bb83a0 <_ZL12overall_size>
(gdb) p estimated_growth - (new_size - old_size)
$3 = -2
(gdb) p predicated
$4 = false
(gdb) p new_size
$5 = 32
(gdb) p old_size
$6 = 22
(gdb) p estimated_growth
$7 = 8

So this is an off-by-two error.  Do multiple INLINE_SIZE_SCALE roundoff
errors not accumulate?
Comment 4 Jakub Jelinek 2012-12-14 10:31:17 UTC
Caused by http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=192821
Comment 5 Richard Biener 2012-12-14 10:42:39 UTC
Happens once in the unreduced testcase.  The following will maybe give us
testcases with even more off values (off-by-1 happens quite often):

Index: gcc/ipa-inline-transform.c
===================================================================
--- gcc/ipa-inline-transform.c  (revision 194496)
+++ gcc/ipa-inline-transform.c  (working copy)
@@ -211,11 +211,6 @@ inline_call (struct cgraph_edge *e, bool
   struct cgraph_node *callee = cgraph_function_or_thunk_node (e->callee, NULL);
   bool new_edges_found = false;
 
-#ifdef ENABLE_CHECKING
-  int estimated_growth = estimate_edge_growth (e);
-  bool predicated = inline_edge_summary (e)->predicate != NULL;
-#endif
-
   /* Don't inline inlined edges.  */
   gcc_assert (e->inline_failed);
   /* Don't even think of inlining inline clone.  */
@@ -263,11 +258,22 @@ inline_call (struct cgraph_edge *e, bool
 #ifdef ENABLE_CHECKING
   /* Verify that estimated growth match real growth.  Allow off-by-one
      error due to INLINE_SIZE_SCALE roudoff errors.  */
-  gcc_assert (!update_overall_summary || !overall_size
-             || abs (estimated_growth - (new_size - old_size)) <= 1
-             /* FIXME: a hack.  Edges with false predicate are accounted
-                wrong, we should remove them from callgraph.  */
-             || predicated);
+  if (update_overall_summary && overall_size)
+    {
+      int estimated_growth = estimate_edge_growth (e);
+      bool predicated = inline_edge_summary (e)->predicate != NULL;
+
+      if (dump_file && (dump_flags & TDF_DETAILS)
+         && (estimated_growth - (new_size - old_size)) != 0)
+       {
+         /* FIXME: Edges with false predicate are accounted
+            wrong, we should remove them from callgraph.  */
+         fprintf (dump_file, " Estimated growth is off by %d%s",
+                  estimated_growth - (new_size - old_size),
+                  predicated ? " (predicated)\n" : "\n");
+       }
+      gcc_assert (abs (estimated_growth - (new_size - old_size)) <= 2);
+    }
 #endif
 
   /* Account the change of overall unit size; external functions will be
Comment 6 Martin Jambor 2012-12-14 12:14:23 UTC
On IRC Richi said that -fno-indirect-inlining helps which would
suggest it is mine (though that still might be coincidence).  For
various reasons, I cannot work on this until Wednesday, I will assign
the bug to me then.  Sorry for the delay, feel free to disable the
check in the meantime.
Comment 7 Richard Biener 2012-12-16 11:58:25 UTC
*** Bug 55711 has been marked as a duplicate of this bug. ***
Comment 8 Richard Biener 2012-12-18 12:46:52 UTC
Bumping the limit to assert on to off-by-two doesn't fix all cases (I can
hand you a testcase if you like...)
Comment 9 Jan Hubicka 2012-12-18 13:39:45 UTC
> Bumping the limit to assert on to off-by-two doesn't fix all cases (I can
> hand you a testcase if you like...)

Yep, i guess it just depends on how many calls we diverge.  I will take a look today after meeting.

Honza
Comment 10 Jan Hubicka 2012-12-18 16:39:47 UTC
OK,
we are inlining
Inline summary for bool C::c1(float, float)/7 inlinable
  self time:       34
  global time:     34
  self size:       18
  global size:     18
  self stack:      0
  global stack:    0
  estimated growth:-6
    size:1.500000, time:1.500000, predicate:(true)
    size:3.500000, time:2.500000, predicate:(not inlined)
    size:0.500000, time:0.500000, predicate:(op0[ref offset: 64] changed) && (not inlined)
    size:0.500000, time:0.500000, predicate:(op0[ref offset: 64] changed)
    size:2.000000, time:2.000000, predicate:(op0[ref offset: 64] changed || op1 changed)
    size:0.500000, time:0.355000, predicate:(op0[ref offset: 96] changed) && (not inlined)
    size:0.500000, time:0.355000, predicate:(op0[ref offset: 96] changed)
    size:2.000000, time:1.420000, predicate:(op0[ref offset: 96] changed || op2 changed)
  calls:
    void B::b2()/12 function body not available
      loop depth: 0 freq: 723 size: 2 time: 11 callee size: 0 stack: 0
    indirect call loop depth: 0 freq:1000 size: 5 time: 17

into 

Inline summary for void bar()/8 inlinable
  self time:       38
  global time:     53
  self size:       22
  global size:     32
  self stack:      0
  global stack:    0
    size:14.500000, time:8.849000, predicate:(true)
    size:3.000000, time:2.000000, predicate:(not inlined)
  calls:
    bool C::c1(float, float)/7 inlined
      loop depth: 0 freq:1000 size: 4 time: 13 callee size: 9 stack: 0
      Stack frame offset 0, callee self size 0, callee size 0
      void B::b2()/12 function body not available
        loop depth: 0 freq: 723 size: 2 time: 11 callee size: 0 stack: 0
      indirect call loop depth: 0 freq:1000 size: 5 time: 17
    double foo()/11 function body not available
      loop depth: 0 freq:1000 size: 2 time: 11 callee size: 0 stack: 0
    void __cxa_guard_release(long long int*)/10 function body not available
      loop depth: 0 freq: 151 size: 2 time: 11 callee size: 0 stack: 0
       op0 is compile time invariant
    int __cxa_guard_acquire(long long int*)/9 function body not available
      loop depth: 0 freq: 389 size: 3 time: 12 callee size: 0 stack: 0
       op0 is compile time invariant

ipa-inline-analysis figures out that we can devirtualize call to b3, but ipa-cp won't.
So this seems another disagreement in between ipa-cp and ipa-prop...

Honza
Comment 11 Jan Hubicka 2012-12-18 17:15:51 UTC
Created attachment 29001 [details]
proposed patch

OK,
we know the argument is constant
 <addr_expr 0x7ffff7585700
    type <pointer_type 0x7ffff75741f8
        type <record_type 0x7ffff7560f18 C sizes-gimplified addressable needs-constructing type_1 type_5 BLK
            size <integer_cst 0x7ffff73f7e40 constant 128>
            unit size <integer_cst 0x7ffff73f7e60 constant 16>
            align 64 symtab 0 alias set 4 canonical type 0x7ffff7560f18 fields <field_decl 0x7ffff755ca18 c2> context <translation_unit_decl 0x7ffff7410170 D.1>
            full-name "struct C"
            needs-constructor X() X(constX&) this=(X&) n_parents=0 use_template=0 interface-unknown
            pointer_to_this <pointer_type 0x7ffff75741f8> chain <type_decl 0x7ffff755aac8 C>>
        public unsigned DI
        size <integer_cst 0x7ffff73f7dc0 constant 64>
        unit size <integer_cst 0x7ffff73f7de0 constant 8>
        align 64 symtab 0 alias set -1 canonical type 0x7ffff75741f8>
    constant
    arg 0 <var_decl 0x7ffff755cda8 c type <record_type 0x7ffff7560f18 C>
        addressable used static tree_1 tree_3 decl_5 decl_6 BLK file t.C line 25 col 12 size <integer_cst 0x7ffff73f7e40 128> unit size <integer_cst 0x7ffff73f7e60 16>
        align 128 context <function_decl 0x7ffff7576900 bar>>>

ipa_get_indirect_edge_target used when we estimate effect of inlining goes into path looking up the binfo based on constant

  if (TREE_CODE (t) != TREE_BINFO)
    { 
      tree binfo;
      binfo = gimple_extract_devirt_binfo_from_cst (t);
      if (!binfo)
        return NULL_TREE;
      binfo = get_binfo_at_offset (binfo, anc_offset, otr_type);
      if (!binfo)
        return NULL_TREE;
      return gimple_get_virt_method_for_binfo (token, binfo);
    }

this code is missing in try_make_edge_direct_virtual_call that is actually responsible for devirtualizing.  I am testing the attached patch.

If I read this right, we should get the problem every time we devirtualize based on static object.  I am surprised this do not trigger in testsuite/bootstrap.

Honza
Comment 12 Jan Hubicka 2012-12-19 11:42:34 UTC
Author: hubicka
Date: Wed Dec 19 11:42:30 2012
New Revision: 194606

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=194606
Log:

	PR tree-optimization/55683
	* g++.dg/ipa/devirt-9.C: New testcase.

	* ipa-prop.c (try_make_edge_direct_virtual_call): Look into constants for binfo.

Added:
    trunk/gcc/testsuite/g++.dg/ipa/devirt-9.C
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/ipa-prop.c
    trunk/gcc/testsuite/ChangeLog
Comment 13 Jan Hubicka 2012-12-19 11:47:06 UTC
The acutal ICE should be fixed.  Martinj, I will leave the PR open just to make you to double check that ipa-cp is doing properly the translation from constants to binfos, too.
I would expect this testcase to be caught by ipa-cp prior inlining. Also I think that when merging values, we should go from constant to binfo when constants differs but their binfos match (so when method is called with multiple static instances of the same object we will get devirtualization).  I don't see ipa-cp doing that.

Honza
Comment 14 Richard Biener 2012-12-19 15:28:30 UTC
Seems to be fixed in all instances I run into.
Comment 15 Martin Jambor 2013-01-10 16:58:35 UTC
(In reply to comment #13)
> The acutal ICE should be fixed.  Martinj, I will leave the PR open
> just to make you to double check that ipa-cp is doing properly the
> translation from constants to binfos, too.

At -O2, IPA-CP does not even consider cloning C::c1 because it is not
allowed to grow code by creating clones.

At -O3 (or with -fipa-cp-clone), IPA-CP discovers that cloning for &c
would lead to devirtualization but because the target of the
devirtualized call is not analyzed, it gets only minimal bonus for
that.  Eventually the cloning opportunity gets score 437 (cloning
threshold is 500) and thus it is dropped.  This is as it should be.

> I would expect this testcase to be caught by ipa-cp prior inlining. Also I
> think that when merging values, we should go from constant to binfo when
> constants differs but their binfos match (so when method is called with
> multiple static instances of the same object we will get devirtualization).  I
> don't see ipa-cp doing that.

Well, the problem with that of course is that we do not merge stuff
now, we accumulate all possible constants.  So what we perhaps should
do instead is (if ipcp_param_lattices->virt_call is true) to try to
see if a number of ADDR_EXPR constants yield the same binfo and if so,
consider that new value first, before any real constants.