From patchwork Thu Mar 7 22:51:14 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 225994 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 002082C03A5 for ; Fri, 8 Mar 2013 09:51:37 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1363301498; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: From:To:Mail-Followup-To:Cc:Subject:Date:Message-ID:User-Agent: MIME-Version:Content-Type:Mailing-List:Precedence:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:Sender: Delivered-To; bh=MfwOKNwyNLLv6j5IniKlrAvUDSs=; b=Eowft0084V+jdUs GMAwUX3AjcV5dXFzrZVmHPnCokguP+ClFGpLCqGnahx5IY/wEswmwkmEzr31c0V4 5brFkReGUu0BZxeZ1PBhT3PCHrcvqZnGMLNuV5vdlD6DJ1cV9XThJs4qrUXkYvpJ Gb1Qrp1F1dvVOj364ARUt/3imnuw= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:X-Received:Received:From:To:Mail-Followup-To:Cc:Subject:Date:Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=G6QreBpkU898wDxiJc4rHCAL5mi2B39WQNLw97qeeQ59OHH/ifTiPWqkgcRf93 7c3N9RkCyKaPZEQAm0GJVo5oRtfR8sRE2yeq2njnZnUHOVliBHdM8yACgdHPjzGy 42O9e389xTiXuD1tWrS40Hteq60q1alHYpLVECu5IlFtc=; Received: (qmail 2560 invoked by alias); 7 Mar 2013 22:51:31 -0000 Received: (qmail 2546 invoked by uid 22791); 7 Mar 2013 22:51:29 -0000 X-SWARE-Spam-Status: No, hits=-4.0 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, KHOP_RCVD_TRUST, KHOP_SPAMHAUS_DROP, RCVD_IN_DNSWL_LOW, RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-wi0-f179.google.com (HELO mail-wi0-f179.google.com) (209.85.212.179) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Thu, 07 Mar 2013 22:51:22 +0000 Received: by mail-wi0-f179.google.com with SMTP id ez12so635686wid.12 for ; Thu, 07 Mar 2013 14:51:20 -0800 (PST) X-Received: by 10.194.119.200 with SMTP id kw8mr57805883wjb.31.1362696680813; Thu, 07 Mar 2013 14:51:20 -0800 (PST) Received: from localhost ([2.26.169.65]) by mx.google.com with ESMTPS id ej8sm36146647wib.9.2013.03.07.14.51.19 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 07 Mar 2013 14:51:20 -0800 (PST) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, jakub@redhat.com, aldyh@redhat.com, sellcey@mips.com, rdsandiford@googlemail.com Cc: jakub@redhat.com, aldyh@redhat.com, sellcey@mips.com Subject: PR 56524: TREE_OPTIMIZATION_OPTABS vs. mips16 Date: Thu, 07 Mar 2013 22:51:14 +0000 Message-ID: <87zjyezuvx.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.2 (gnu/linux) MIME-Version: 1.0 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 This assert in save_optabs_if_changed triggered for gcc.dg/pr43564.c on mips64-linux-gnu (-mabi=32/-mips16): /* ?? If this fails, we should temporarily restore the default target first (set_cfun (NULL) ??), do the rest of this function, and then restore it. */ gcc_assert (this_target_optabs == &default_target_optabs); We can't do what the comment says because set_cfun (NULL) might not choose default_target_optabs. (On MIPS, default_target_optabs is always for the standard encoding, in order to reduce the differences between the -mips16 command-line option and "mips16" function attribute.) The patch instead records which value of this_target_optabs was used to generate TREE_OPTIMIZATION_OPTABS. This has the additional advantage that we won't recompute the optabs if successive functions use the same optimisation node and same non-default target (which, given the above, is relatively likely with -mips16). Recording the base this_target_optabs also lets us initialise TREE_OPTIMIZATION_OPTABS lazily, so that the C front end doesn't need to compute optabs when parsing the "optimize" attribute. That's probably only a minor benefit though. After this change, cfun->optabs is only useful on switchable targets if we regularly switch between functions that have the same non-default optimisation node and _different_ subtargets. This seems like a relatively unusual case so I removed cfun->optabs for simplicity. However, Jakub didn't think that was a good idea, and since "simplicity" is a bit of weak argument (especially as Jakub says for something that's already implemented), I was going to submit two alternatives, one that kept cfun->optabs and one that didn't. However, I think there are two other benefits to removing cfun->optabs: - It means that the TREE_OPTIMIZATION_OPTABS are unequivocally owned by the optimisation node, rather than shared between the node and the function structure. It's therefore safe to clear the old optabs when we want to recompute them. - The current implementation of the cache is: struct function *fn = DECL_STRUCT_FUNCTION (fndecl); if (fn->optabs == NULL) { if (this_target_optabs == &default_target_optabs) fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); else { fn->optabs = (unsigned char *) ggc_alloc_atomic (sizeof (struct target_optabs)); init_all_optabs ((struct target_optabs *) fn->optabs); } } this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs : this_target_optabs; The equivalent code under the new scheme would be: struct function *fn = DECL_STRUCT_FUNCTION (fndecl); if (fn->optabs == NULL) { init_tree_optimization_optabs (opts); fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); } this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs : this_target_optabs; But this wouldn't help if the optimisation node does not need different optabs from this_target_optabs. fn->optabs would remain null and we'd still go through init_tree_optimization_optabs each time. And I think that's almost always going to be the case with optabs on MIPS, the only switchable target. I think the only optabs that depend on flags are the floating-point division patterns, which depend on flag_unsafe_math_optimizations when using -mfix-sb1. But the SB-1 doesn't have MIPS16 support, so it's unlikely that you'd use the two together. And that's building on top of what already seems an unlikely situation. Assuming I've not missed some patterns (which is a very risky assumption), I'm not sure anyone would benefit in practice. We could add a flag to say whether cfun->optabs is valid, but that's adding even more complexity. Tested on x86_64-linux-gnu and mips64-linux-gnu. OK to install? Richard gcc/ PR middle-end/56524 * tree.h (tree_optimization_option): Rename target_optabs to optabs. Add base_optabs. (TREE_OPTIMIZATION_OPTABS): Update after previous field change. (TREE_OPTIMIZATION_BASE_OPTABS): New macro. (save_optabs_if_changed): Replace with... (init_tree_optimization_optabs): ...this. * optabs.c (save_optabs_if_changed): Rename to... (init_tree_optimization_optabs): ...this. Take the optimization node as argument. Do nothing if the base optabs are already correct. * function.h (function): Remove optabs field. * function.c (invoke_set_current_function_hook): Call init_tree_optimization_optabs. Use the result to initialize this_fn_optabs. gcc/c-family/ PR middle-end/56524 * c-common.c (handle_optimize_attribute): Don't call save_optabs_if_changed. gcc/testsuite/ PR middle-end/56524 * gcc.target/mips/pr56524.c: New test. Index: gcc/tree.h =================================================================== --- gcc/tree.h 2013-03-07 19:23:42.019041307 +0000 +++ gcc/tree.h 2013-03-07 19:42:05.427263768 +0000 @@ -3589,21 +3589,26 @@ struct GTY(()) tree_optimization_option /* Target optabs for this set of optimization options. This is of type `struct target_optabs *'. */ - unsigned char *GTY ((atomic)) target_optabs; + unsigned char *GTY ((atomic)) optabs; + + /* The value of this_target_optabs against which the optabs above were + generated. */ + struct target_optabs *GTY ((skip)) base_optabs; }; #define TREE_OPTIMIZATION(NODE) \ (&OPTIMIZATION_NODE_CHECK (NODE)->optimization.opts) #define TREE_OPTIMIZATION_OPTABS(NODE) \ - (OPTIMIZATION_NODE_CHECK (NODE)->optimization.target_optabs) + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.optabs) + +#define TREE_OPTIMIZATION_BASE_OPTABS(NODE) \ + (OPTIMIZATION_NODE_CHECK (NODE)->optimization.base_optabs) /* Return a tree node that encapsulates the current optimization options. */ extern tree build_optimization_node (void); -/* Save a new set of target_optabs in a TREE_OPTIMIZATION node if the - current set of optabs has changed. */ -extern void save_optabs_if_changed (tree); +extern void init_tree_optimization_optabs (tree); /* Target options used by a function. */ Index: gcc/optabs.c =================================================================== --- gcc/optabs.c 2013-03-07 19:23:42.019041307 +0000 +++ gcc/optabs.c 2013-03-07 19:42:05.426263760 +0000 @@ -6208,19 +6208,25 @@ init_optabs (void) targetm.init_libfuncs (); } -/* Recompute the optabs and save them if they have changed. */ +/* Use the current target and options to initialize + TREE_OPTIMIZATION_OPTABS (OPTNODE). */ void -save_optabs_if_changed (tree fndecl) +init_tree_optimization_optabs (tree optnode) { - /* ?? If this fails, we should temporarily restore the default - target first (set_cfun (NULL) ??), do the rest of this function, - and then restore it. */ - gcc_assert (this_target_optabs == &default_target_optabs); + /* Quick exit if we have already computed optabs for this target. */ + if (TREE_OPTIMIZATION_BASE_OPTABS (optnode) == this_target_optabs) + return; + /* Forget any previous information and set up for the current target. */ + TREE_OPTIMIZATION_BASE_OPTABS (optnode) = this_target_optabs; struct target_optabs *tmp_optabs = (struct target_optabs *) - ggc_alloc_atomic (sizeof (struct target_optabs)); - tree optnode = DECL_FUNCTION_SPECIFIC_OPTIMIZATION (fndecl); + TREE_OPTIMIZATION_OPTABS (optnode); + if (tmp_optabs) + memset (tmp_optabs, 0, sizeof (struct target_optabs)); + else + tmp_optabs = (struct target_optabs *) + ggc_alloc_atomic (sizeof (struct target_optabs)); /* Generate a new set of optabs into tmp_optabs. */ init_all_optabs (tmp_optabs); Index: gcc/function.h =================================================================== --- gcc/function.h 2013-03-07 19:23:42.019041307 +0000 +++ gcc/function.h 2013-03-07 19:42:05.425263752 +0000 @@ -580,9 +580,6 @@ struct GTY(()) function { a string describing the reason for failure. */ const char * GTY((skip)) cannot_be_copied_reason; - /* Optabs for this function. This is of type `struct target_optabs *'. */ - unsigned char *GTY ((atomic)) optabs; - /* Collected bit flags. */ /* Number of units of general registers that need saving in stdarg Index: gcc/function.c =================================================================== --- gcc/function.c 2013-03-07 19:23:42.019041307 +0000 +++ gcc/function.c 2013-03-07 19:42:05.425263752 +0000 @@ -4400,25 +4400,14 @@ invoke_set_current_function_hook (tree f } targetm.set_current_function (fndecl); + this_fn_optabs = this_target_optabs; - if (opts == optimization_default_node) - this_fn_optabs = this_target_optabs; - else + if (opts != optimization_default_node) { - struct function *fn = DECL_STRUCT_FUNCTION (fndecl); - if (fn->optabs == NULL) - { - if (this_target_optabs == &default_target_optabs) - fn->optabs = TREE_OPTIMIZATION_OPTABS (opts); - else - { - fn->optabs = (unsigned char *) - ggc_alloc_atomic (sizeof (struct target_optabs)); - init_all_optabs ((struct target_optabs *) fn->optabs); - } - } - this_fn_optabs = fn->optabs ? (struct target_optabs *) fn->optabs - : this_target_optabs; + init_tree_optimization_optabs (opts); + if (TREE_OPTIMIZATION_OPTABS (opts)) + this_fn_optabs = (struct target_optabs *) + TREE_OPTIMIZATION_OPTABS (opts); } } } Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c 2013-03-07 19:23:42.019041307 +0000 +++ gcc/c-family/c-common.c 2013-03-07 19:42:05.424263744 +0000 @@ -8947,8 +8947,6 @@ handle_optimize_attribute (tree *node, t DECL_FUNCTION_SPECIFIC_OPTIMIZATION (*node) = build_optimization_node (); - save_optabs_if_changed (*node); - /* Restore current options. */ cl_optimization_restore (&global_options, &cur_opts); } Index: gcc/testsuite/gcc.target/mips/pr56524.c =================================================================== --- /dev/null 2013-03-06 23:07:14.594799386 +0000 +++ gcc/testsuite/gcc.target/mips/pr56524.c 2013-03-07 19:43:01.038692028 +0000 @@ -0,0 +1,8 @@ +/* { dg-options "-mips16" } */ + +void bar (void) {} + +void __attribute__((optimize("schedule-insns"))) +foo (void) +{ +}