From patchwork Sat Aug 2 10:07:21 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jan Hubicka X-Patchwork-Id: 375933 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]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id F354D1400DD for ; Sat, 2 Aug 2014 20:07:38 +1000 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; q=dns; s= default; b=cTRL8vy+9/M49dQBvTKZMZxaJDjWa5TFYd8t0VHFyspV3BycoYIa1 fFiLSuM105Qy6PcOjRguSTdHxeV1H+rlxkZTpN7w+rGOmCgc0UzTtGzbPahrgrHz 9KCC1kDsp5/xnW+qDN4PDi+9/ybUwyQF4b2o+/oMDveWfhR2nwbGL4= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:subject:message-id:mime-version:content-type; s= default; bh=GamdNs0oKp19YbixglkBsuXd6YQ=; b=ykah2TUKT8l7rTGYc3rF nnI5kUm51jsH05deyiQ3TO7r4BXbDeb//TpAyT9VXgvFzpsVXrhN/nLkCmZoN+UK 0j9rOEp0w94/7AlyRScvJGgFgA/+/S+MVNcMZJNK103sJUQldOGGRyfhMxYY/S1h PKDThL+lVCVMm+KWIKLJy38= Received: (qmail 8703 invoked by alias); 2 Aug 2014 10:07:30 -0000 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 Received: (qmail 8676 invoked by uid 89); 2 Aug 2014 10:07:29 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.7 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: nikam.ms.mff.cuni.cz Received: from nikam.ms.mff.cuni.cz (HELO nikam.ms.mff.cuni.cz) (195.113.20.16) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Sat, 02 Aug 2014 10:07:25 +0000 Received: by nikam.ms.mff.cuni.cz (Postfix, from userid 16202) id 723125419E2; Sat, 2 Aug 2014 12:07:21 +0200 (CEST) Date: Sat, 2 Aug 2014 12:07:21 +0200 From: Jan Hubicka To: gcc-patches@gcc.gnu.org Subject: Add -Wsuggest-final-warnings and -Wsuggest-final-methods warnings Message-ID: <20140802100721.GB12987@kam.mff.cuni.cz> MIME-Version: 1.0 Content-Disposition: inline User-Agent: Mutt/1.5.21 (2010-09-15) Hi, this patch adds warnings -Wsuggest-final-types and -Wsuggest-final-methods that output warnings when adding C++ final keyword would enable devirtualizations. This warning is expected to be taken as a hint by the developers who can annotate sources where it makes sense from design standpoint. The warning works both with and without LTO, but it is a lot more accurate with LTO. About 8000 warings are output for libxul build, so I added code sorting them by importance. Type final specifiers are suggested first, method later (because making type final makes method annotations unnecesary). Warnings output for firefox are placed here http://kam.mff.cuni.cz/~hubicka/final-warnings.txt I can also add warnings for all types with no derived types and all virtual methods with no overriders, but I am not sure if that would be of any use. The patch also speeds up the type inheritance analysis that got quite slow on Firefox after introducing speculative contextes (we now derrive a lot of speculative info we didn't before). I think i will need to reorganize how speculation is handled andseparate fll and speculative lists as one tends to be long and others short but many. Bootstrapped/regtested x86_64-linux, I am retesting after a last minute change and intend to commit soon. Honza * doc/invoke.texi (Wsuggest-final-types, Wsuggest-final-methods): Document. * ipa-devirt.c: Include hash-map.h (struct polymorphic_call_target_d): Add type_warning and decl_warning. (clear_speculation): Break out of ... (get_class_context): ... here; speed up handling obviously useless speculations. (odr_type_warn_count, decl_warn_count): New structures. (final_warning_record): New structure. (final_warning_records): New static variable. (possible_polymorphic_call_targets): Cleanup handling of speculative info; do not build speculation when user do not care; record info about warnings when asked for. (add_decl_warning): New function. (type_warning_cmp): New function. (decl_warning_cmp): New function. (ipa_devirt): Handle -Wsuggest-final-methods and -Wsuggest-final-types. (gate): Enable pass when warnings are requested. * common.opt (Wsuggest-final-types, Wsuggest-final-methods): New options. * g++.dg/warn/Wsuggest-final.C: New testcase. * g++.dg/ipa/devirt-34.C: Fix. Index: doc/invoke.texi =================================================================== --- doc/invoke.texi (revision 213406) +++ doc/invoke.texi (working copy) @@ -271,6 +271,7 @@ Objective-C and Objective-C++ Dialects}. -Wstack-protector -Wstack-usage=@var{len} -Wstrict-aliasing @gol -Wstrict-aliasing=n @gol -Wstrict-overflow -Wstrict-overflow=@var{n} @gol -Wsuggest-attribute=@r{[}pure@r{|}const@r{|}noreturn@r{|}format@r{]} @gol +-Wsuggest-final-types @gol -Wsuggest-final-methods @gol -Wmissing-format-attribute @gol -Wswitch -Wswitch-default -Wswitch-enum -Wswitch-bool -Wsync-nand @gol -Wsystem-headers -Wtrampolines -Wtrigraphs -Wtype-limits -Wundef @gol @@ -4193,6 +4194,25 @@ case, and some functions for which @code appropriate may not be detected. @end table +@item -Wsuggest-final-types +@opindex Wno-suggest-final-types +@opindex Wsuggest-final-types +Warn about types with virtual methods where code quality would be improved +if the type was declared with C++11 final specifier, or, if possible, +declared in anonymous namespace. This allows GCC to devritualize more aggressively +the polymorphic calls. This warning is more effective with link time optimization, +where the information about the class hiearchy is more complete. + +@item -Wsuggest-final-methods +@opindex Wno-suggest-final-methods +@opindex Wsuggest-final-methods +Warn about virtual methods where code quality would be improved if the method +was declared with C++11 final specifier, or, if possible, its type was declared +in the anonymous namespace or with final specifier. This warning is more +effective with link time optimization, where the information about the class +hiearchy graph is more complete. It is recommended to first consider suggestions +of @option{-Wsuggest-final-types} and then rebuild with new annotations. + @item -Warray-bounds @opindex Wno-array-bounds @opindex Warray-bounds @@ -9601,7 +9621,7 @@ before applying @option{--param inline-u @item inline-unit-growth Specifies maximal overall growth of the compilation unit caused by inlining. The default value is 30 which limits unit growth to 1.3 times the original -size. Cold functions (either marked cold via an attribibute or by profile +size. Cold functions (either marked cold via an attribute or by profile feedback) are not accounted into the unit size. @item ipcp-unit-growth Index: testsuite/g++.dg/warn/Wsuggest-final.C =================================================================== --- testsuite/g++.dg/warn/Wsuggest-final.C (revision 0) +++ testsuite/g++.dg/warn/Wsuggest-final.C (revision 0) @@ -0,0 +1,14 @@ +// { dg-do compile } +// { dg-options "-Wsuggest-final-types -Wsuggest-final-methods" } +struct A { // { dg-warning "final would enable devirtualization of 4 calls" "correct warning" } +virtual void a() {} // { dg-warning "final would enable devirtualization of 2 calls" "correct warning" } + virtual void b() {} // { dg-warning "final would enable devirtualization of 2 calls" "correct warning" } +}; +void +t(struct A *a) +{ + a->a(); + a->a(); + a->b(); + a->b(); +} Index: testsuite/g++.dg/ipa/devirt-34.C =================================================================== --- testsuite/g++.dg/ipa/devirt-34.C (revision 213406) +++ testsuite/g++.dg/ipa/devirt-34.C (working copy) @@ -2,6 +2,9 @@ /* { dg-options "-O2 -fdump-ipa-devirt" } */ struct A {virtual int t(){return 42;}}; struct B:A {virtual int t(){return 1;}}; + +struct A aa; +struct B bb; int t(struct B *b) { Index: ipa-devirt.c =================================================================== --- ipa-devirt.c (revision 213406) +++ ipa-devirt.c (working copy) @@ -133,6 +133,7 @@ along with GCC; see the file COPYING3. #include "dbgcnt.h" #include "stor-layout.h" #include "intl.h" +#include "hash-map.h" static bool odr_types_equivalent_p (tree, tree, bool, bool *, pointer_set_t *); @@ -1617,6 +1618,8 @@ struct polymorphic_call_target_d vec targets; int speculative_targets; bool complete; + int type_warning; + tree decl_warning; }; /* Polymorphic call target cache helpers. */ @@ -1735,6 +1738,16 @@ contains_polymorphic_type_p (const_tree return false; } +/* Clear speculative info from CONTEXT. */ + +static void +clear_speculation (ipa_polymorphic_call_context *context) +{ + context->speculative_outer_type = NULL; + context->speculative_offset = 0; + context->speculative_maybe_derived_type = false; +} + /* CONTEXT->OUTER_TYPE is a type of memory object where object of EXPECTED_TYPE is contained at CONTEXT->OFFSET. Walk the memory representation of CONTEXT->OUTER_TYPE and find the outermost class type that match @@ -1770,6 +1783,16 @@ get_class_context (ipa_polymorphic_call_ type = context->outer_type = expected_type; context->offset = offset = 0; } + + if (context->speculative_outer_type == context->outer_type + && (!context->maybe_derived_type + || context->speculative_maybe_derived_type)) + { + context->speculative_outer_type = NULL; + context->speculative_offset = 0; + context->speculative_maybe_derived_type = false; + } + /* See if speculative type seem to be derrived from outer_type. Then speculation is valid only if it really is a derivate and derived types are allowed. @@ -1786,11 +1809,7 @@ get_class_context (ipa_polymorphic_call_ context->outer_type)) speculation_valid = context->maybe_derived_type; else - { - context->speculative_outer_type = NULL; - context->speculative_offset = 0; - context->speculative_maybe_derived_type = false; - } + clear_speculation (context); /* Find the sub-object the constant actually refers to and mark whether it is an artificial one (as opposed to a user-defined one). @@ -1821,10 +1840,7 @@ get_class_context (ipa_polymorphic_call_ context->outer_type) && (context->maybe_derived_type == context->speculative_maybe_derived_type))) - { - context->speculative_outer_type = NULL; - context->speculative_offset = 0; - } + clear_speculation (context); return true; } else @@ -1842,8 +1858,7 @@ get_class_context (ipa_polymorphic_call_ if (!speculation_valid || !context->maybe_derived_type) { - context->speculative_outer_type = NULL; - context->speculative_offset = 0; + clear_speculation (context); return true; } /* Otherwise look into speculation now. */ @@ -1926,9 +1941,7 @@ get_class_context (ipa_polymorphic_call_ /* If we failed to find subtype we look for, give up and fall back to the most generic query. */ give_up: - context->speculative_outer_type = NULL; - context->speculative_offset = 0; - context->speculative_maybe_derived_type = false; + clear_speculation (context); if (valid) return true; context->outer_type = expected_type; @@ -2503,6 +2516,30 @@ devirt_variable_node_removal_hook (varpo free_polymorphic_call_targets_hash (); } +/* Record about how many calls would benefit from given type to be final. */ +struct odr_type_warn_count +{ + int count; + gcov_type dyn_count; +}; + +/* Record about how many calls would benefit from given method to be final. */ +struct decl_warn_count +{ + tree decl; + int count; + gcov_type dyn_count; +}; + +/* Information about type and decl warnings. */ +struct final_warning_record +{ + gcov_type dyn_count; + vec type_warnings; + hash_map decl_warnings; +}; +struct final_warning_record *final_warning_records; + /* Return vector containing possible targets of polymorphic call of type OTR_TYPE caling method OTR_TOKEN within type of OTR_OUTER_TYPE and OFFSET. If INCLUDE_BASES is true, walk also base types of OUTER_TYPES containig @@ -2576,6 +2613,10 @@ possible_polymorphic_call_targets (tree return nodes; } + /* Do not bother to compute speculative info when user do not asks for it. */ + if (!speculative_targetsp || !context.speculative_outer_type) + clear_speculation (&context); + type = get_odr_type (otr_type, true); /* Recording type variants would wast results cache. */ @@ -2642,6 +2683,19 @@ possible_polymorphic_call_targets (tree *completep = (*slot)->complete; if (speculative_targetsp) *speculative_targetsp = (*slot)->speculative_targets; + if ((*slot)->type_warning && final_warning_records) + { + final_warning_records->type_warnings[(*slot)->type_warning - 1].count++; + final_warning_records->type_warnings[(*slot)->type_warning - 1].dyn_count + += final_warning_records->dyn_count; + } + if ((*slot)->decl_warning && final_warning_records) + { + struct decl_warn_count *c = + final_warning_records->decl_warnings.get ((*slot)->decl_warning); + c->count++; + c->dyn_count += final_warning_records->dyn_count; + } return (*slot)->targets; } @@ -2660,9 +2714,13 @@ possible_polymorphic_call_targets (tree inserted = pointer_set_create (); matched_vtables = pointer_set_create (); + /* First insert targets we speculatively identified as likely. */ if (context.speculative_outer_type) { odr_type speculative_outer_type; + bool speculation_complete = true; + + /* First insert target from type itself and check if it may have derived types. */ speculative_outer_type = get_odr_type (context.speculative_outer_type, true); if (TYPE_FINAL_P (speculative_outer_type->type)) context.speculative_maybe_derived_type = false; @@ -2674,35 +2732,27 @@ possible_polymorphic_call_targets (tree else target = NULL; - if (target) - { - /* In the case we get complete method, we don't need - to walk derivations. */ - if (DECL_FINAL_P (target)) - context.speculative_maybe_derived_type = false; - } + /* In the case we get complete method, we don't need + to walk derivations. */ + if (target && DECL_FINAL_P (target)) + context.speculative_maybe_derived_type = false; if (type_possibly_instantiated_p (speculative_outer_type->type)) - maybe_record_node (nodes, target, inserted, can_refer, &complete); + maybe_record_node (nodes, target, inserted, can_refer, &speculation_complete); if (binfo) pointer_set_insert (matched_vtables, BINFO_VTABLE (binfo)); + /* Next walk recursively all derived types. */ if (context.speculative_maybe_derived_type) - { - /* For anonymous namespace types we can attempt to build full type. - All derivations must be in this unit (unless we see partial unit). */ - if (!type->all_derivations_known) - complete = false; - for (i = 0; i < speculative_outer_type->derived_types.length(); i++) - possible_polymorphic_call_targets_1 (nodes, inserted, - matched_vtables, - otr_type, - speculative_outer_type->derived_types[i], - otr_token, speculative_outer_type->type, - context.speculative_offset, &complete, - bases_to_consider, - false); - } - /* Finally walk bases, if asked to. */ + for (i = 0; i < speculative_outer_type->derived_types.length(); i++) + possible_polymorphic_call_targets_1 (nodes, inserted, + matched_vtables, + otr_type, + speculative_outer_type->derived_types[i], + otr_token, speculative_outer_type->type, + context.speculative_offset, + &speculation_complete, + bases_to_consider, + false); (*slot)->speculative_targets = nodes.length(); } @@ -2746,10 +2796,6 @@ possible_polymorphic_call_targets (tree /* Next walk recursively all derived types. */ if (context.maybe_derived_type) { - /* For anonymous namespace types we can attempt to build full type. - All derivations must be in this unit (unless we see partial unit). */ - if (!type->all_derivations_known) - complete = false; for (i = 0; i < outer_type->derived_types.length(); i++) possible_polymorphic_call_targets_1 (nodes, inserted, matched_vtables, @@ -2759,6 +2805,51 @@ possible_polymorphic_call_targets (tree context.offset, &complete, bases_to_consider, context.maybe_in_construction); + + if (!outer_type->all_derivations_known) + { + if (final_warning_records) + { + if (complete + && nodes.length () == 1 + && warn_suggest_final_types + && !outer_type->derived_types.length ()) + { + if (outer_type->id >= (int)final_warning_records->type_warnings.length ()) + final_warning_records->type_warnings.safe_grow_cleared + (odr_types.length ()); + final_warning_records->type_warnings[outer_type->id].count++; + final_warning_records->type_warnings[outer_type->id].dyn_count + += final_warning_records->dyn_count; + (*slot)->type_warning = outer_type->id + 1; + } + if (complete + && warn_suggest_final_methods + && nodes.length () == 1 + && types_same_for_odr (DECL_CONTEXT (nodes[0]->decl), + outer_type->type)) + { + bool existed; + struct decl_warn_count &c = + final_warning_records->decl_warnings.get_or_insert + (nodes[0]->decl, &existed); + + if (existed) + { + c.count++; + c.dyn_count += final_warning_records->dyn_count; + } + else + { + c.count = 1; + c.dyn_count = final_warning_records->dyn_count; + c.decl = nodes[0]->decl; + } + (*slot)->decl_warning = nodes[0]->decl; + } + } + complete = false; + } } /* Finally walk bases, if asked to. */ @@ -2801,6 +2892,14 @@ possible_polymorphic_call_targets (tree return nodes; } +bool +add_decl_warning (const tree &key ATTRIBUTE_UNUSED, const decl_warn_count &value, + vec *vec) +{ + vec->safe_push (&value); + return true; +} + /* Dump all possible targets of a polymorphic call. */ void @@ -2951,6 +3050,38 @@ likely_target_p (struct cgraph_node *n) return true; } +/* Compare type warning records P1 and P2 and chose one with larger count; + helper for qsort. */ + +int +type_warning_cmp (const void *p1, const void *p2) +{ + const odr_type_warn_count *t1 = (const odr_type_warn_count *)p1; + const odr_type_warn_count *t2 = (const odr_type_warn_count *)p2; + + if (t1->dyn_count < t2->dyn_count) + return 1; + if (t1->dyn_count > t2->dyn_count) + return -1; + return t2->count - t1->count; +} + +/* Compare decl warning records P1 and P2 and chose one with larger count; + helper for qsort. */ + +int +decl_warning_cmp (const void *p1, const void *p2) +{ + const decl_warn_count *t1 = *(const decl_warn_count * const *)p1; + const decl_warn_count *t2 = *(const decl_warn_count * const *)p2; + + if (t1->dyn_count < t2->dyn_count) + return 1; + if (t1->dyn_count > t2->dyn_count) + return -1; + return t2->count - t1->count; +} + /* The ipa-devirt pass. When polymorphic call has only one likely target in the unit, turn it into speculative call. */ @@ -2966,6 +3097,19 @@ ipa_devirt (void) int nmultiple = 0, noverwritable = 0, ndevirtualized = 0, nnotdefined = 0; int nwrong = 0, nok = 0, nexternal = 0, nartificial = 0; + /* We can output -Wsuggest-final-methods and -Wsuggest-final-types warnings. + This is implemented by setting up final_warning_records that are updated + by get_polymorphic_call_targets. + We need to clear cache in this case to trigger recomputation of all + entries. */ + if (warn_suggest_final_methods || warn_suggest_final_types) + { + final_warning_records = new (final_warning_record); + final_warning_records->type_warnings = vNULL; + final_warning_records->type_warnings.safe_grow_cleared (odr_types.length ()); + free_polymorphic_call_targets_hash (); + } + FOR_EACH_DEFINED_FUNCTION (n) { bool update = false; @@ -2979,6 +3123,10 @@ ipa_devirt (void) void *cache_token; bool final; int speculative_targets; + + if (final_warning_records) + final_warning_records->dyn_count = e->count; + vec targets = possible_polymorphic_call_targets (e, &final, &cache_token, &speculative_targets); @@ -2990,6 +3138,9 @@ ipa_devirt (void) npolymorphic++; + if (!flag_devirtualize_speculatively) + continue; + if (!cgraph_maybe_hot_edge_p (e)) { if (dump_file) @@ -3121,6 +3272,55 @@ ipa_devirt (void) inline_update_overall_summary (n); } pointer_set_destroy (bad_call_targets); + if (warn_suggest_final_methods || warn_suggest_final_types) + { + if (warn_suggest_final_types) + { + final_warning_records->type_warnings.qsort (type_warning_cmp); + for (unsigned int i = 0; + i < final_warning_records->type_warnings.length (); i++) + if (final_warning_records->type_warnings[i].count) + { + odr_type type = odr_types[i]; + warning_at (DECL_SOURCE_LOCATION (TYPE_NAME (type->type)), + OPT_Wsuggest_final_types, + "Declaring type %qD final " + "would enable devirtualization of %i calls", + type->type, + final_warning_records->type_warnings[i].count); + } + } + + if (warn_suggest_final_methods) + { + vec decl_warnings_vec = vNULL; + + final_warning_records->decl_warnings.traverse + *, add_decl_warning> (&decl_warnings_vec); + decl_warnings_vec.qsort (decl_warning_cmp); + for (unsigned int i = 0; i < decl_warnings_vec.length (); i++) + { + tree decl = decl_warnings_vec[i]->decl; + int count = decl_warnings_vec[i]->count; + + if (DECL_CXX_DESTRUCTOR_P (decl)) + warning_at (DECL_SOURCE_LOCATION (decl), + OPT_Wsuggest_final_methods, + "Declaring virtual destructor of %qD final " + "would enable devirtualization of %i calls", + DECL_CONTEXT (decl), count); + else + warning_at (DECL_SOURCE_LOCATION (decl), + OPT_Wsuggest_final_methods, + "Declaring method %qD final " + "would enable devirtualization of %i calls", + decl, count); + } + } + + delete (final_warning_records); + final_warning_records = 0; + } if (dump_file) fprintf (dump_file, @@ -3170,7 +3370,9 @@ public: virtual bool gate (function *) { return (flag_devirtualize - && flag_devirtualize_speculatively + && (flag_devirtualize_speculatively + || (warn_suggest_final_methods + || warn_suggest_final_types)) && optimize); } Index: common.opt =================================================================== --- common.opt (revision 213406) +++ common.opt (working copy) @@ -651,6 +651,14 @@ Wsuggest-attribute=noreturn Common Var(warn_suggest_attribute_noreturn) Warning Warn about functions which might be candidates for __attribute__((noreturn)) +Wsuggest-final-types +Common Var(warn_suggest_final_types) Warning +Warn about C++ polymorphic types where adding final keyword would improve code quality + +Wsuggest-final-methods +Common Var(warn_suggest_final_methods) Warning +Warn about C++ virtual methods where adding final keyword would improve code quality + Wsystem-headers Common Var(warn_system_headers) Warning Do not suppress warnings from system headers