On Thu, Jul 16, 2015 at 04:21:05PM +0100, Kyrill Tkachov wrote:
> Hi all,
>
> This patch implements target pragmas for aarch64.
> The pragmas accepted are the same as for target attributes (as required).
> In addition pragmas will need to redefine the target-specific preprocessor
> macros if appropriate.
>
> A new file: aarch64-c.c is added and the code from TARGET_CPU_CPP_BUILTINS is
> moved there
> and split up into the unconditional parts that are always defined and the
> conditional stuff
> that depends on certain architectural features. The pragma processing code
> calls that
> to redefine preprocessor macros on the fly.
> The implementation is similar to the rs6000 one.
>
> With target pragmas implemented, we can use them in the arm_neon.h and
> arm_acle.h headers to
> specify the architectural features required for those intrinsics, rather than
> #ifdef'ing them
> out when FP/SIMD is not available from the command line.
>
> We need to do this in order to handle cases where the user compiles a file
> with -mgeneral-regs-only
> but has a function tagged with +simd and tries to use the arm_neon.h
> intrinsics.
> Tests and documentation comes as a separate patch later on in the series
>
> Bootstrapped and tested on aarch64.
>
> Ok for trunk?
A couple of ChangeLog nits and some comments below.
>
> 2015-07-16 Kyrylo Tkachov <kyrylo.tkac...@arm.com>
>
> * config.gcc (aarch64*-*-*): Specify c_target_objs and cxx_target_objs.
> * config/aarch64/aarch64.h (REGISTER_TARGET_PRAGMAS):
This should say
> * config/aarch64/aarch64.h (REGISTER_TARGET_PRAGMAS): New.
Presumably (or maybe "Define.").
> (TARGET_CPU_CPP_BUILTINS): Redefine to call aarch64_cpu_cpp_builtins.
> * config/aarch64/aarch64.c (aarch64_override_options_internal): Remove
> static keyword.
> (aarch64_reset_previous_fndecl): New function.
> * config/aarch64/aarch64-c.c: New file.
> * config/aarch64/arm_acle.h: Add pragma +crc+nofp at the top.
> Push and pop options at beginning and end. Remove ifdef
> __ARM_FEATURE_CRC32.
> * config/aarch64/arm_neon.h: Remove #ifdef check on __ARM_NEON.
> Add pragma arch=armv8-a+simd and +crypto where appropriate.
> * config/aarch64/t-aarch64 (aarch64-c.o): New rule.
I don't see a ChangeLog entry for these hunks:
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 3a5482d..4704736 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -360,6 +360,10 @@ bool aarch64_gen_adjusted_ldpstp (rtx *, bool, enum
> machine_mode, RTX_CODE);
> #endif /* RTX_CODE */
>
> void aarch64_init_builtins (void);
> +
> +bool aarch64_process_target_attr (tree, const char*);
> +void aarch64_override_options_internal (struct gcc_options *);
> +
> rtx aarch64_expand_builtin (tree exp,
> rtx target,
> rtx subtarget ATTRIBUTE_UNUSED,
> @@ -376,6 +380,9 @@ extern void aarch64_split_combinev16qi (rtx operands[3]);
> extern void aarch64_expand_vec_perm (rtx target, rtx op0, rtx op1, rtx sel);
> extern bool aarch64_madd_needs_nop (rtx_insn *);
> extern void aarch64_final_prescan_insn (rtx_insn *);
> +extern void aarch64_reset_previous_fndecl (void);
> +extern void aarch64_cpu_cpp_builtins (cpp_reader *);
> +extern void aarch64_register_pragmas (void);
> 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 *,
> +static bool
> +aarch64_pragma_target_parse (tree args, tree pop_target)
> +{
> +
> + bool ret;
> +
> + /* If args is not NULL then process it and setup the target-specific
> + information that it specifies. */
> + if (args)
> + {
> + ret = aarch64_process_target_attr (args, "pragma");
> + if (ret)
> + aarch64_override_options_internal (&global_options);
RET must equal true.
> + else
> + return false;
Early return of false closes the other control path here.
> + }
> +
> + /* args is NULL, restore to the state described in pop_target. */
> + else
> + {
> + pop_target = pop_target ? pop_target : target_option_default_node;
> + cl_target_option_restore (&global_options,
> + TREE_TARGET_OPTION (pop_target));
> + ret = true;
> + }
Therefore RET must equal true here.
> +
> + target_option_current_node
> + = build_target_option_node (&global_options);
> +
> + aarch64_reset_previous_fndecl ();
> + /* For the definitions, ensure all newly defined macros are considered
> + as used for -Wunused-macros. There is no point warning about the
> + compiler predefined macros. */
> + cpp_options *cpp_opts = cpp_get_options (parse_in);
> + unsigned char saved_warn_unused_macros = cpp_opts->warn_unused_macros;
> + cpp_opts->warn_unused_macros = 0;
> +
> + aarch64_update_cpp_builtins (parse_in);
> +
> + cpp_opts->warn_unused_macros = saved_warn_unused_macros;
> +
> + return ret;
So we don't need "RET" !
> +}
> +
> +/* Implement REGISTER_TARGET_PRAGMAS. */
> +
> +void
> +aarch64_register_pragmas (void)
> +{
> + /* Update pragma hook to allow parsing #pragma GCC target. */
> + targetm.target_option.pragma_parse = aarch64_pragma_target_parse;
> +}
> \ No newline at end of file
I can't remember if GNU style mandates it, but in my opinion your new
file should have a trailing newline.
> diff --git a/gcc/config/aarch64/aarch64-protos.h
> b/gcc/config/aarch64/aarch64-protos.h
> index 3a5482d..4704736 100644
> --- a/gcc/config/aarch64/aarch64-protos.h
> +++ b/gcc/config/aarch64/aarch64-protos.h
> @@ -360,6 +360,10 @@ bool aarch64_gen_adjusted_ldpstp (rtx *, bool, enum
> machine_mode, RTX_CODE);
> #endif /* RTX_CODE */
>
> void aarch64_init_builtins (void);
> +
> +bool aarch64_process_target_attr (tree, const char*);
> +void aarch64_override_options_internal (struct gcc_options *);
> +
> rtx aarch64_expand_builtin (tree exp,
> rtx target,
> rtx subtarget ATTRIBUTE_UNUSED,
> @@ -376,6 +380,9 @@ extern void aarch64_split_combinev16qi (rtx operands[3]);
> extern void aarch64_expand_vec_perm (rtx target, rtx op0, rtx op1, rtx sel);
> extern bool aarch64_madd_needs_nop (rtx_insn *);
> extern void aarch64_final_prescan_insn (rtx_insn *);
> +extern void aarch64_reset_previous_fndecl (void);
> +extern void aarch64_cpu_cpp_builtins (cpp_reader *);
> +extern void aarch64_register_pragmas (void);
At one point aarch64-protos.h was in alphabetical order. While we have
a number of mistakes already, we should try not to make the situation
worse!
> diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
> index 7af4ad2..f6b2c11 100644
> --- a/gcc/config/aarch64/arm_acle.h
> +++ b/gcc/config/aarch64/arm_acle.h
> @@ -28,11 +28,16 @@
> #define _GCC_ARM_ACLE_H
>
> #include <stdint.h>
> +
> +#pragma GCC push_options
> +/* Add +nofp to make sure that 'fp' is not required to compile these
> + intrinsics. */
> +#pragma GCC target("+crc+nofp")
Hm, how does this work with the ARMv8.1 Extensions added by Matthew
Wahab recently? Presumably this needs to expand to have a "+no" for
all possible extensions. This seems messy, it might be neater to
implement something like +nothing which resets the state of the extension
features bitmask to zero.
> +
> #ifdef __cplusplus
> extern "C" {
> #endif
>
> -#ifdef __ARM_FEATURE_CRC32
> __extension__ static __inline uint32_t __attribute__ ((__always_inline__))
> __crc32b (uint32_t __a, uint8_t __b)
> {
> @@ -81,10 +86,10 @@ __crc32d (uint32_t __a, uint64_t __b)
> return __builtin_aarch64_crc32x (__a, __b);
> }
>
> -#endif
> -
> #ifdef __cplusplus
> }
> #endif
>
> +#pragma GCC pop_options
> +
> #endif
Thanks,
James
