From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 11092 invoked by alias); 4 Aug 2015 08:53:55 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 11082 invoked by uid 89); 4 Aug 2015 08:53:55 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 X-HELO: cam-smtp0.cambridge.arm.com Received: from fw-tnat.cambridge.arm.com (HELO cam-smtp0.cambridge.arm.com) (217.140.96.140) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Tue, 04 Aug 2015 08:53:53 +0000 Received: from arm.com (e106375-lin.cambridge.arm.com [10.2.207.23]) by cam-smtp0.cambridge.arm.com (8.13.8/8.13.8) with ESMTP id t748romR009485; Tue, 4 Aug 2015 09:53:50 +0100 Date: Tue, 04 Aug 2015 08:53:00 -0000 From: James Greenhalgh To: Kyrill Tkachov Cc: GCC Patches , Marcus Shawcroft , Richard Earnshaw Subject: Re: [PATCH][AArch64][8/14] Implement TARGET_OPTION_VALID_ATTRIBUTE_P Message-ID: <20150804085350.GA10636@arm.com> References: <55A7CBDB.9020908@arm.com> <20150721153733.GA13935@arm.com> <55B216D4.5010604@arm.com> <20150803105246.GA10382@arm.com> <55BF86AD.2020701@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55BF86AD.2020701@arm.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes X-SW-Source: 2015-08/txt/msg00143.txt.bz2 On Mon, Aug 03, 2015 at 04:20:13PM +0100, Kyrill Tkachov wrote: > Ok, I've removed usages of 'ret' in favor of returning when appropriate. > In this last one I left the ret (but cleaned up the control flow a bit) > because if the processing fails we need to clean up a bit of state before > returning. This is OK with the changes below fixed, or commented on as justification. > diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h > index fc1cec7..3a5482d 100644 > --- a/gcc/config/aarch64/aarch64-protos.h > +++ b/gcc/config/aarch64/aarch64-protos.h > @@ -376,6 +378,8 @@ extern bool aarch64_madd_needs_nop (rtx_insn *); > extern void aarch64_final_prescan_insn (rtx_insn *); > extern bool > aarch64_expand_vec_perm_const (rtx target, rtx op0, rtx op1, rtx sel); > +bool aarch64_handle_option (struct gcc_options *, struct gcc_options *, > + const struct cl_decoded_option *, location_t); Please try to keep this file in alphabetical order, first by return type, then by function name. > void aarch64_atomic_assign_expand_fenv (tree *, tree *, tree *); > int aarch64_ccmp_mode_to_code (enum machine_mode mode); > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index d0d62e7..7a369fd 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > +static bool > +aarch64_handle_attr_arch (const char *str, const char *pragma_or_attr) > +{ > + const struct processor *tmp_arch = NULL; > + enum aarch64_parse_opt_result parse_res > + = aarch64_parse_arch (str, &tmp_arch, &aarch64_isa_flags); > + > + if (parse_res == AARCH64_PARSE_OK) > + { > + gcc_assert (tmp_arch); > + selected_arch = tmp_arch; > + explicit_arch = selected_arch->arch; > + return true; > + } Why not pull this in to the switch case below? > + > + switch (parse_res) > + { > + case AARCH64_PARSE_MISSING_ARG: > + error ("missing architecture name in 'arch' target %s", pragma_or_attr); > + break; > + case AARCH64_PARSE_INVALID_ARG: > + error ("unknown value %qs for 'arch' target %s", str, pragma_or_attr); > + break; > + case AARCH64_PARSE_INVALID_FEATURE: > + error ("invalid feature modifier %qs for 'arch' target %s", > + str, pragma_or_attr); > + break; > + default: > + gcc_unreachable (); > + } > + > + return false; > +} > + > +/* Handle the argument CPU_STR to the cpu= target attribute. > + PRAGMA_OR_ATTR is used in potential error messages. */ > + > +static bool > +aarch64_handle_attr_cpu (const char *str, const char *pragma_or_attr) > +{ > + const struct processor *tmp_cpu = NULL; > + enum aarch64_parse_opt_result parse_res > + = aarch64_parse_cpu (str, &tmp_cpu, &aarch64_isa_flags); > + > + if (parse_res == AARCH64_PARSE_OK) > + { > + gcc_assert (tmp_cpu); > + selected_tune = tmp_cpu; > + explicit_tune_core = selected_tune->ident; > + > + selected_arch = &all_architectures[tmp_cpu->arch]; > + explicit_arch = selected_arch->arch; > + return true; > + } Likewise here. > + > + switch (parse_res) > + { > + case AARCH64_PARSE_MISSING_ARG: > + error ("missing cpu name in 'cpu' target %s", pragma_or_attr); > + break; > + case AARCH64_PARSE_INVALID_ARG: > + error ("unknown value %qs for 'cpu' target %s", str, pragma_or_attr); > + break; > + case AARCH64_PARSE_INVALID_FEATURE: > + error ("invalid feature modifier %qs for 'cpu' target %s", > + str, pragma_or_attr); > + break; > + default: > + gcc_unreachable (); > + } > + > + return false; > +} > + > +/* Handle the argument STR to the tune= target attribute. > + PRAGMA_OR_ATTR is used in potential error messages. */ > + > +static bool > +aarch64_handle_attr_tune (const char *str, const char *pragma_or_attr) > +{ > + const struct processor *tmp_tune = NULL; > + enum aarch64_parse_opt_result parse_res > + = aarch64_parse_tune (str, &tmp_tune); > + > + if (parse_res == AARCH64_PARSE_OK) > + { > + gcc_assert (tmp_tune); > + selected_tune = tmp_tune; > + explicit_tune_core = selected_tune->ident; > + return true; > + } > + And likewise here. > + switch (parse_res) > + { > + case AARCH64_PARSE_INVALID_ARG: > + error ("unknown value %qs for 'tune' target %s", str, pragma_or_attr); > + break; > + default: > + gcc_unreachable (); > + } > + > + return false; > +} > + > +/* Parse an architecture extensions target attribute string specified in STR. > + For example "+fp+nosimd". Show any errors if needed. Return TRUE > + if successful. Update aarch64_isa_flags to reflect the ISA features > + modified. > + PRAGMA_OR_ATTR is used in potential error messages. */ > + > +static bool > +aarch64_handle_attr_isa_flags (char *str, const char *pragma_or_attr) > +{ > + enum aarch64_parse_opt_result parse_res; > + unsigned long isa_flags = aarch64_isa_flags; > + > + parse_res = aarch64_parse_extension (str, &isa_flags); > + > + if (parse_res == AARCH64_PARSE_OK) > + { > + aarch64_isa_flags = isa_flags; > + return true; > + } > + And again here. > + switch (parse_res) > + { > + case AARCH64_PARSE_MISSING_ARG: > + error ("missing feature modifier in target %s %qs", > + pragma_or_attr, str); > + break; > + > + case AARCH64_PARSE_INVALID_FEATURE: > + error ("invalid feature modifier in target %s %qs", > + pragma_or_attr, str); > + break; > + > + default: > + gcc_unreachable (); > + } > + > + return false; > +} > + Thanks, James