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
