From patchwork Wed Sep 22 18:30:57 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Martin Jambor X-Patchwork-Id: 65441 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 7FADDB70E1 for ; Thu, 23 Sep 2010 04:31:15 +1000 (EST) Received: (qmail 5269 invoked by alias); 22 Sep 2010 18:31:13 -0000 Received: (qmail 5244 invoked by uid 22791); 22 Sep 2010 18:31:11 -0000 X-SWARE-Spam-Status: No, hits=-6.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_HI X-Spam-Check-By: sourceware.org Received: from cantor.suse.de (HELO mx1.suse.de) (195.135.220.2) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 22 Sep 2010 18:31:00 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.221.2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.suse.de (Postfix) with ESMTP id 259438E8CC; Wed, 22 Sep 2010 20:30:58 +0200 (CEST) Date: Wed, 22 Sep 2010 20:30:57 +0200 From: Martin Jambor To: Jan Hubicka Cc: Richard Guenther , GCC Patches Subject: Re: [PATCH, PR 45572] Fix two minor issues with indirect inlining Message-ID: <20100922183057.GA31138@virgil.arch.suse.de> Mail-Followup-To: Jan Hubicka , Richard Guenther , GCC Patches References: <20100922095156.GA3994@virgil.arch.suse.de> <20100922102018.GP6006@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20100922102018.GP6006@kam.mff.cuni.cz> User-Agent: Mutt/1.5.20 (2009-06-14) X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Hi, On Wed, Sep 22, 2010 at 12:20:18PM +0200, Jan Hubicka wrote: > > On Wed, Sep 22, 2010 at 11:51 AM, Martin Jambor wrote: > > > Hi, > > > > > > PR 45572 has three testcases which exhibit two different bugs: > > > > > > 1) ipa_make_edge_direct_to_target can inadvertently lazily create new > > > cgraph nodes when looking up a node for a decl with cgraph_node > > > function.  This node then has an uid which is larger than size of > > > ipa_node_params_vector, causing index out of bounds assert.  The > > > function should not create new nodes and so I replaced the call with a > > > call to cgraph_get_node. > > > > That part sounds obvious, you might want to install it separately. > > This is the case where we have previously virtual call but > devirtualization finds call using object with external vtable (so > function never seen previously becomes reachable). > > I don't like the approach of keeping calls indirect when they are > really known to be direct. Even when it won't degrade inlining > (since the function was unseen previously and thus has no body), > still how much pain would be to update tables instead? It is as easy as calling ipa_check_create_node_params after cgraph_node(). I just did not want to reallocate the vectors needlessly. The new patch below does that. > > > > > > 2) If recursive indirect edges are discovered during recursive > > > inlining, they are added to new_edges but > > > cgraph_decide_recursive_inlining then looks them itself and processes > > > them immediately.  When edges from new_edges are processed, it > > > attempts to inline recursively them again which leads to all sorts of > > > weird consequences.  Fixed by teaching the indirect inlining machinery > > > not to add recursive edges when doing recursive inlining, which is > > > flagged by new parameters of ipa_propagate_indirect_call_infos and > > > unfortunately also of cgraph_mark_inline_edge. > > > > Can you expand on the "weird consequences"? Could we instead > > disable indirect inlining during recursive inlining (and would that > > make the patch any prettier?). > > I saw here a testcase where virtual method was self recursive. Only after > inlining the first call the self recursiveness become obvious, so I guess > we should handle this. > > I looked into the PR briefly before and noticed it has an weird consequences, > but didn't quite understood why? > Yesterday I had a theory what was going on but I was wrong. The problem was simply that in add_new_edges_to_heap we did: if (edge->callee->local.inlinable && cgraph_default_inline_p (edge->callee, &edge->inline_failed)) which overwrote edge->inline_failed with a non-zero, making an already inlined edge an un-inlined one. I think this was then usually directly caught by the verifier but potentially other mischief could happen, like redirecting such edges to master_clones, resulting in dangling inlined_to nodes and so on and so forth. It took me quite a while to realize what was going on, the line looked so innocent. Anyway, here is a much simpler patch which also works. Bootstrapped and tested on x86_64-linux. OK for trunk? 2010-09-22 Martin Jambor PR tree-optimization/45572 * ipa-prop.c (ipa_make_edge_direct_to_target): Call ipa_check_create_node_params. * ipa-inline.c (add_new_edges_to_heap): Do not insert inlined edges. * testsuite/g++.dg/ipa/pr45572-1.C: New test. * testsuite/g++.dg/ipa/pr45572-2.C: Likewise. Index: icln/gcc/testsuite/g++.dg/ipa/pr45572-1.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/pr45572-1.C @@ -0,0 +1,64 @@ +// { dg-do compile } +// { dg-options "-finline-small-functions -findirect-inlining -finline-functions -O" } + +extern "C" { +typedef long unsigned int size_t; +typedef long int __ssize_t; +typedef struct _IO_FILE FILE; +typedef struct +{ +} __mbstate_t; +extern __inline __attribute__ ((__gnu_inline__)) int +fgetc_unlocked (FILE *__fp) +{ +} +extern __inline __attribute__ ((__gnu_inline__)) int +putc_unlocked (int __c, FILE *__stream) +{ +} +extern __inline __attribute__ ((__gnu_inline__)) __ssize_t +getline (char **__lineptr, size_t *__n, FILE *__stream) +{ +} +extern __inline __attribute__ ((__gnu_inline__)) int +ferror_unlocked (FILE *__stream) throw () +{ +} +} +typedef struct +{} __mpf_struct; +typedef __mpf_struct mpf_t[1]; +typedef const __mpf_struct *mpf_srcptr; +typedef __mpf_struct *mpf_ptr; +extern "C" { + void __gmpf_add (mpf_ptr, mpf_srcptr, mpf_srcptr); +} +class _knumber +{ + public: + enum NumType {SpecialType, IntegerType, FractionType, FloatType}; + virtual NumType type(void) const = 0; + virtual _knumber * add(_knumber const & arg2) const = 0; + virtual operator long int(void) const = 0; +}; +class _knumfloat : public _knumber +{ + _knumfloat(double num = 1.0) + ; + virtual NumType type(void) const ; + virtual _knumber * add(_knumber const & arg2) const; + virtual operator long int (void) const; + mpf_t _mpf; +}; +_knumber *_knumfloat::add(_knumber const & arg2) const +{ + if (arg2.type() == SpecialType) + return arg2.add(*this); +{ + _knumfloat tmp_num(arg2); + return tmp_num.add(*this); + } + _knumfloat * tmp_num = new _knumfloat(); + __gmpf_add(tmp_num->_mpf, _mpf, + dynamic_cast<_knumfloat const &>(arg2)._mpf); +} Index: icln/gcc/testsuite/g++.dg/ipa/pr45572-2.C =================================================================== --- /dev/null +++ icln/gcc/testsuite/g++.dg/ipa/pr45572-2.C @@ -0,0 +1,39 @@ +// { dg-do compile } +// { dg-options "-finline-small-functions -findirect-inlining -finline-function+ +typedef struct +{} __mpf_struct; +typedef __mpf_struct mpf_t[1]; +typedef const __mpf_struct *mpf_srcptr; +typedef __mpf_struct *mpf_ptr; +extern "C" { + void __gmpf_add (mpf_ptr, mpf_srcptr, mpf_srcptr); +} +class _knumber +{ + public: + enum NumType {SpecialType, IntegerType, FractionType, FloatType}; + virtual NumType type(void) const = 0; + virtual _knumber * add(_knumber const & arg2) const = 0; + virtual operator long int(void) const = 0; +}; +class _knumfloat : public _knumber +{ + _knumfloat(double num = 1.0) + ; + virtual NumType type(void) const ; + virtual _knumber * add(_knumber const & arg2) const; + virtual operator long int (void) const; + mpf_t _mpf; +}; +_knumber *_knumfloat::add(_knumber const & arg2) const +{ + if (arg2.type() == SpecialType) + return arg2.add(*this); +{ + _knumfloat tmp_num(arg2); + return tmp_num.add(*this); + } + _knumfloat * tmp_num = new _knumfloat(); + __gmpf_add(tmp_num->_mpf, _mpf, + dynamic_cast<_knumfloat const &>(arg2)._mpf); +} Index: icln/gcc/ipa-inline.c =================================================================== --- icln.orig/gcc/ipa-inline.c +++ icln/gcc/ipa-inline.c @@ -1031,6 +1031,7 @@ add_new_edges_to_heap (fibheap_t heap, V gcc_assert (!edge->aux); if (edge->callee->local.inlinable + && edge->inline_failed && cgraph_default_inline_p (edge->callee, &edge->inline_failed)) edge->aux = fibheap_insert (heap, cgraph_edge_badness (edge, false), edge); } Index: icln/gcc/ipa-prop.c =================================================================== --- icln.orig/gcc/ipa-prop.c +++ icln/gcc/ipa-prop.c @@ -1447,7 +1447,7 @@ ipa_make_edge_direct_to_target (struct c callee = cgraph_node (target); if (!callee) return NULL; - + ipa_check_create_node_params (); cgraph_make_edge_direct (ie, callee); if (dump_file) {