* [Patch AArch64 2/3] Rework the code to print extension strings (pr70133)
2016-04-06 10:10 [Patch AArch64 0/3] Fix PR70133 James Greenhalgh
@ 2016-04-06 10:10 ` James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 1/3] Enable CRC by default for armv8.1-a James Greenhalgh
` (2 subsequent siblings)
3 siblings, 0 replies; 9+ messages in thread
From: James Greenhalgh @ 2016-04-06 10:10 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, marcus.shawcroft, richard.earnshaw
[-- Attachment #1: Type: text/plain, Size: 2815 bytes --]
Hi,
This patch aims to ensure that when we say:
-march=armv8-a+nosimd
We communicate that to the assembler in a way it understands.
On trunk, we'll put out a directive that says
.march armv8-a+fp
Which has two issues; it is not enough to disable the simd extensions, and
it adds a +fp that is already implied by the .march. Rather, we should
be emitting:
.march armv8-a+nosimd
Fixing this is reasonably easy. The size of this patch comes from some
refactoring to move aarch64_parse_extension from config/aarch64/aarch64.c
to common/config/aarch64/aarch64-common.c . we also need to modify
config/aarch64/aarch64-option-extensions.def to start keeping track of
a canonical name for each flag. Doing this means updating config.gcc with
a new regex.
We also need to fixup two testcases that look for an incorrect .arch
directive in the assembler output.
We need this to rationalise the way we put out .arch strings in
preparation for the next patch in the series, and to fix the existing bugs
and deficiencies in our .arch handling.
As an exception +crc is only sometimes guaranteed to be enabled by default
for ARMv8.1-A, so we always have to put this out.
Bootstrapped on aarch64-none-linux-gnu and tested for the defaults, and
with an explicit -march=native passed to dejagnu.
OK?
Thanks,
James
---
gcc/
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
PR target/70133
* config/aarch64/aarch64-common.c (aarch64_option_extension): Keep
track of a canonical flag name.
(all_extensions): Likewise.
(arch_to_arch_name): Also track extension flags enabled by the arch.
(all_architectures): Likewise.
(aarch64_parse_extension): Move to here.
(aarch64_get_extension_string_for_isa_flags): Take a new argument,
rework.
(aarch64_rewrite_selected_cpu): Update for above change.
* config/aarch64/aarch64-option-extensions.def: Rework the way flags
are handled, such that the single explicit value enabled by an
extension is kept seperate from the implicit values it also enables.
* config/aarch64/aarch64-protos.h (aarch64_parse_opt_result): Move
to here.
(aarch64_parse_extension): New.
* config/aarch64/aarch64.c (aarch64_parse_opt_result): Move from
here to config/aarch64/aarch64-protos.h.
(aarch64_parse_extension): Move from here to
common/config/aarch64/aarch64-common.c.
(aarch64_option_print): Update.
(aarch64_declare_function_name): Likewise.
(aarch64_start_file): Likewise.
* config/aarch64/driver-aarch64.c (arch_extension): Keep track of
the canonical flag for extensions.
* config.gcc (aarch64*-*-*): Extend regex for capturing extension
flags.
gcc/testsuite/
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
PR target/70133
* gcc.target/aarch64/mgeneral-regs_4.c: Fix expected output.
* gcc.target/aarch64/target_attr_15.c: Likewise.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-Patch-AArch64-2-3-Rework-the-code-to-print-extension.patch --]
[-- Type: text/x-patch; name="0002-Patch-AArch64-2-3-Rework-the-code-to-print-extension.patch", Size: 19338 bytes --]
diff --git a/gcc/common/config/aarch64/aarch64-common.c b/gcc/common/config/aarch64/aarch64-common.c
index 4969f07..08e7959 100644
--- a/gcc/common/config/aarch64/aarch64-common.c
+++ b/gcc/common/config/aarch64/aarch64-common.c
@@ -112,6 +112,7 @@ struct gcc_targetm_common targetm_common = TARGETM_COMMON_INITIALIZER;
struct aarch64_option_extension
{
const char *const name;
+ const unsigned long flag_canonical;
const unsigned long flags_on;
const unsigned long flags_off;
};
@@ -119,11 +120,11 @@ struct aarch64_option_extension
/* ISA extensions in AArch64. */
static const struct aarch64_option_extension all_extensions[] =
{
-#define AARCH64_OPT_EXTENSION(NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
- {NAME, FLAGS_ON, FLAGS_OFF},
+#define AARCH64_OPT_EXTENSION(NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, Z) \
+ {NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF},
#include "config/aarch64/aarch64-option-extensions.def"
#undef AARCH64_OPT_EXTENSION
- {NULL, 0, 0}
+ {NULL, 0, 0, 0}
};
struct processor_name_to_arch
@@ -137,6 +138,7 @@ struct arch_to_arch_name
{
const enum aarch64_arch arch;
const std::string arch_name;
+ const unsigned long flags;
};
/* Map processor names to the architecture revision they implement and
@@ -155,26 +157,111 @@ static const struct processor_name_to_arch all_cores[] =
static const struct arch_to_arch_name all_architectures[] =
{
#define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH, FLAGS) \
- {AARCH64_ARCH_##ARCH_IDENT, NAME},
+ {AARCH64_ARCH_##ARCH_IDENT, NAME, FLAGS},
#include "config/aarch64/aarch64-arches.def"
#undef AARCH64_ARCH
- {aarch64_no_arch, ""}
+ {aarch64_no_arch, "", 0}
};
-/* Return a string representation of ISA_FLAGS. */
+/* Parse the architecture extension string STR and update ISA_FLAGS
+ with the architecture features turned on or off. Return a
+ aarch64_parse_opt_result describing the result. */
+
+enum aarch64_parse_opt_result
+aarch64_parse_extension (const char *str, unsigned long *isa_flags)
+{
+ /* The extension string is parsed left to right. */
+ const struct aarch64_option_extension *opt = NULL;
+
+ /* Flag to say whether we are adding or removing an extension. */
+ int adding_ext = -1;
+
+ while (str != NULL && *str != 0)
+ {
+ const char *ext;
+ size_t len;
+
+ str++;
+ ext = strchr (str, '+');
+
+ if (ext != NULL)
+ len = ext - str;
+ else
+ len = strlen (str);
+
+ if (len >= 2 && strncmp (str, "no", 2) == 0)
+ {
+ adding_ext = 0;
+ len -= 2;
+ str += 2;
+ }
+ else if (len > 0)
+ adding_ext = 1;
+
+ if (len == 0)
+ return AARCH64_PARSE_MISSING_ARG;
+
+
+ /* Scan over the extensions table trying to find an exact match. */
+ for (opt = all_extensions; opt->name != NULL; opt++)
+ {
+ if (strlen (opt->name) == len && strncmp (opt->name, str, len) == 0)
+ {
+ /* Add or remove the extension. */
+ if (adding_ext)
+ *isa_flags |= (opt->flags_on | opt->flag_canonical);
+ else
+ *isa_flags &= ~(opt->flags_off | opt->flag_canonical);
+ break;
+ }
+ }
+
+ if (opt->name == NULL)
+ {
+ /* Extension not found in list. */
+ return AARCH64_PARSE_INVALID_FEATURE;
+ }
+
+ str = ext;
+ };
+
+ return AARCH64_PARSE_OK;
+}
+
+/* Return a string representation of ISA_FLAGS. DEFAULT_ARCH_FLAGS
+ gives the default set of flags which are implied by whatever -march
+ we'd put out. Our job is to figure out the minimal set of "+" and
+ "+no" feature flags to put out, and to put them out grouped such
+ that all the "+" flags come before the "+no" flags. */
std::string
-aarch64_get_extension_string_for_isa_flags (unsigned long isa_flags)
+aarch64_get_extension_string_for_isa_flags (unsigned long isa_flags,
+ unsigned long default_arch_flags)
{
const struct aarch64_option_extension *opt = NULL;
std::string outstr = "";
+ /* Pass one: Find all the things we need to turn on. As a special case,
+ we always want to put out +crc if it is enabled. */
for (opt = all_extensions; opt->name != NULL; opt++)
- if ((isa_flags & opt->flags_on) == opt->flags_on)
+ if ((isa_flags & opt->flag_canonical
+ && !(default_arch_flags & opt->flag_canonical))
+ || (default_arch_flags & opt->flag_canonical
+ && opt->flag_canonical == AARCH64_ISA_CRC))
{
outstr += "+";
outstr += opt->name;
}
+
+ /* Pass two: Find all the things we need to turn off. */
+ for (opt = all_extensions; opt->name != NULL; opt++)
+ if ((~isa_flags) & opt->flag_canonical
+ && !((~default_arch_flags) & opt->flag_canonical))
+ {
+ outstr += "+no";
+ outstr += opt->name;
+ }
+
return outstr;
}
@@ -186,7 +273,7 @@ const char *
aarch64_rewrite_selected_cpu (const char *name)
{
std::string original_string (name);
- std::string extensions;
+ std::string extension_str;
std::string processor;
size_t extension_pos = original_string.find_first_of ('+');
@@ -194,8 +281,8 @@ aarch64_rewrite_selected_cpu (const char *name)
if (extension_pos != std::string::npos)
{
processor = original_string.substr (0, extension_pos);
- extensions = original_string.substr (extension_pos,
- std::string::npos);
+ extension_str = original_string.substr (extension_pos,
+ std::string::npos);
}
else
{
@@ -227,9 +314,12 @@ aarch64_rewrite_selected_cpu (const char *name)
|| a_to_an->arch == aarch64_no_arch)
fatal_error (input_location, "unknown value %qs for -mcpu", name);
+ unsigned long extensions = p_to_a->flags;
+ aarch64_parse_extension (extension_str.c_str (), &extensions);
+
std::string outstr = a_to_an->arch_name
- + aarch64_get_extension_string_for_isa_flags (p_to_a->flags)
- + extensions;
+ + aarch64_get_extension_string_for_isa_flags (extensions,
+ a_to_an->flags);
/* We are going to memory leak here, nobody elsewhere
in the callchain is going to clean up after us. The alternative is
diff --git a/gcc/config.gcc b/gcc/config.gcc
index 6722260..f66e48c 100644
--- a/gcc/config.gcc
+++ b/gcc/config.gcc
@@ -3620,22 +3620,28 @@ case "${target}" in
${srcdir}/config/aarch64/aarch64-option-extensions.def \
> /dev/null; then
- ext_on=`grep "^AARCH64_OPT_EXTENSION(\"$base_ext\"," \
+ ext_canon=`grep "^AARCH64_OPT_EXTENSION(\"$base_ext\"," \
${srcdir}/config/aarch64/aarch64-option-extensions.def | \
sed -e 's/^[^,]*,[ ]*//' | \
sed -e 's/,.*$//'`
- ext_off=`grep "^AARCH64_OPT_EXTENSION(\"$base_ext\"," \
+ ext_on=`grep "^AARCH64_OPT_EXTENSION(\"$base_ext\"," \
${srcdir}/config/aarch64/aarch64-option-extensions.def | \
sed -e 's/^[^,]*,[ ]*[^,]*,[ ]*//' | \
sed -e 's/,.*$//' | \
sed -e 's/).*$//'`
+ ext_off=`grep "^AARCH64_OPT_EXTENSION(\"$base_ext\"," \
+ ${srcdir}/config/aarch64/aarch64-option-extensions.def | \
+ sed -e 's/^[^,]*,[ ]*[^,]*,[ ]*[^,]*,[ ]*//' | \
+ sed -e 's/,.*$//' | \
+ sed -e 's/).*$//'`
+
if [ $ext = $base_ext ]; then
# Adding extension
- ext_mask="("$ext_mask") | ("$ext_on")"
+ ext_mask="("$ext_mask") | ("$ext_on" | "$ext_canon")"
else
# Removing extension
- ext_mask="("$ext_mask") & ~("$ext_off")"
+ ext_mask="("$ext_mask") & ~("$ext_off" | "$ext_canon")"
fi
true
diff --git a/gcc/config/aarch64/aarch64-option-extensions.def b/gcc/config/aarch64/aarch64-option-extensions.def
index fbf9a53..e8706d1 100644
--- a/gcc/config/aarch64/aarch64-option-extensions.def
+++ b/gcc/config/aarch64/aarch64-option-extensions.def
@@ -21,23 +21,37 @@
Before using #include to read this file, define a macro:
- AARCH64_OPT_EXTENSION(EXT_NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING)
+ AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, FEATURE_STRING)
EXT_NAME is the name of the extension, represented as a string constant.
- FLAGS_ON are the bitwise-or of the features that the extension adds.
- FLAGS_OFF are the bitwise-or of the features that the extension removes.
+ FLAGS_CANONICAL is the canonical internal name for this flag.
+ FLAGS_ON are the bitwise-or of the features that enabling the extension
+ adds, or zero if enabling this extension has no effect on other features.
+ FLAGS_OFF are the bitwise-or of the features that disabling the extension
+ removes, or zero if disabling this extension has no effect on other
+ features.
FEAT_STRING is a string containing the entries in the 'Features' field of
/proc/cpuinfo on a GNU/Linux system that correspond to this architecture
extension being available. Sometimes multiple entries are needed to enable
the extension (for example, the 'crypto' extension depends on four
entries: aes, pmull, sha1, sha2 being present). In that case this field
- should contain a whitespace-separated list of the strings in 'Features'
+ should contain a space (" ") separated list of the strings in 'Features'
that are required. Their order is not important. */
-AARCH64_OPT_EXTENSION ("fp", AARCH64_FL_FP,
- AARCH64_FL_FPSIMD | AARCH64_FL_CRYPTO, "fp")
-AARCH64_OPT_EXTENSION ("simd", AARCH64_FL_FPSIMD,
- AARCH64_FL_SIMD | AARCH64_FL_CRYPTO, "asimd")
-AARCH64_OPT_EXTENSION("crypto", AARCH64_FL_CRYPTO | AARCH64_FL_FPSIMD, AARCH64_FL_CRYPTO, "aes pmull sha1 sha2")
-AARCH64_OPT_EXTENSION("crc", AARCH64_FL_CRC, AARCH64_FL_CRC, "crc32")
-AARCH64_OPT_EXTENSION("lse", AARCH64_FL_LSE, AARCH64_FL_LSE, "atomics")
+/* Enabling "fp" just enables "fp".
+ Disabling "fp" also disables "simd", "crypto". */
+AARCH64_OPT_EXTENSION("fp", AARCH64_FL_FP, 0, AARCH64_FL_SIMD | AARCH64_FL_CRYPTO, "fp")
+
+/* Enabling "simd" also enables "fp".
+ Disabling "simd" also disables "crypto". */
+AARCH64_OPT_EXTENSION("simd", AARCH64_FL_SIMD, AARCH64_FL_FP, AARCH64_FL_CRYPTO, "asimd")
+
+/* Enabling "crypto" also enables "fp", "simd".
+ Disabling "crypto" just disables "crypto". */
+AARCH64_OPT_EXTENSION("crypto", AARCH64_FL_CRYPTO, AARCH64_FL_FP | AARCH64_FL_SIMD, 0, "aes pmull sha1 sha2")
+
+/* Enabling or disabling "crc" only changes "crc". */
+AARCH64_OPT_EXTENSION("crc", AARCH64_FL_CRC, 0, 0, "crc32")
+
+/* Enabling or disabling "lse" only changes "lse". */
+AARCH64_OPT_EXTENSION("lse", AARCH64_FL_LSE, 0, 0, "atomics")
diff --git a/gcc/config/aarch64/aarch64-protos.h b/gcc/config/aarch64/aarch64-protos.h
index 58c9d0d..f22a31c 100644
--- a/gcc/config/aarch64/aarch64-protos.h
+++ b/gcc/config/aarch64/aarch64-protos.h
@@ -263,6 +263,18 @@ enum aarch64_extra_tuning_flags
};
#undef AARCH64_EXTRA_TUNING_OPTION
+/* Enum describing the various ways that the
+ aarch64_parse_{arch,tune,cpu,extension} functions can fail.
+ This way their callers can choose what kind of error to give. */
+
+enum aarch64_parse_opt_result
+{
+ AARCH64_PARSE_OK, /* Parsing was successful. */
+ AARCH64_PARSE_MISSING_ARG, /* Missing argument. */
+ AARCH64_PARSE_INVALID_FEATURE, /* Invalid feature modifier. */
+ AARCH64_PARSE_INVALID_ARG /* Invalid arch, tune, cpu arg. */
+};
+
extern struct tune_params aarch64_tune_params;
HOST_WIDE_INT aarch64_initial_elimination_offset (unsigned, unsigned);
@@ -280,8 +292,6 @@ bool aarch64_float_const_zero_rtx_p (rtx);
bool aarch64_function_arg_regno_p (unsigned);
bool aarch64_gen_movmemqi (rtx *);
bool aarch64_gimple_fold_builtin (gimple_stmt_iterator *);
-bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
- const struct cl_decoded_option *, location_t);
bool aarch64_is_extend_from_extract (machine_mode, rtx, rtx);
bool aarch64_is_long_call_p (rtx);
bool aarch64_is_noplt_call_p (rtx);
@@ -315,7 +325,6 @@ bool aarch64_uimm12_shift (HOST_WIDE_INT);
bool aarch64_use_return_insn_p (void);
const char *aarch64_mangle_builtin_type (const_tree);
const char *aarch64_output_casesi (rtx *);
-const char *aarch64_rewrite_selected_cpu (const char *name);
enum aarch64_symbol_type aarch64_classify_symbol (rtx, rtx);
enum aarch64_symbol_type aarch64_classify_tls_symbol (rtx);
@@ -338,7 +347,6 @@ rtx aarch64_simd_gen_const_vector_dup (machine_mode, int);
bool aarch64_simd_mem_operand_p (rtx);
rtx aarch64_simd_vect_par_cnst_half (machine_mode, bool);
rtx aarch64_tls_get_addr (void);
-std::string aarch64_get_extension_string_for_isa_flags (unsigned long);
tree aarch64_fold_builtin (tree, int, tree *, bool);
unsigned aarch64_dbx_register_number (unsigned);
unsigned aarch64_trampoline_size (void);
@@ -433,4 +441,13 @@ extern bool aarch64_nopcrelative_literal_loads;
extern void aarch64_asm_output_pool_epilogue (FILE *, const char *,
tree, HOST_WIDE_INT);
+/* Defined in common/config/aarch64-common.c. */
+bool aarch64_handle_option (struct gcc_options *, struct gcc_options *,
+ const struct cl_decoded_option *, location_t);
+const char *aarch64_rewrite_selected_cpu (const char *name);
+enum aarch64_parse_opt_result aarch64_parse_extension (const char *,
+ unsigned long *);
+std::string aarch64_get_extension_string_for_isa_flags (unsigned long,
+ unsigned long);
+
#endif /* GCC_AARCH64_PROTOS_H */
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index b7086dd..9995494 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -666,7 +666,7 @@ struct aarch64_option_extension
/* ISA extensions in AArch64. */
static const struct aarch64_option_extension all_extensions[] =
{
-#define AARCH64_OPT_EXTENSION(NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
+#define AARCH64_OPT_EXTENSION(NAME, X, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
{NAME, FLAGS_ON, FLAGS_OFF},
#include "aarch64-option-extensions.def"
#undef AARCH64_OPT_EXTENSION
@@ -7673,83 +7673,6 @@ aarch64_add_stmt_cost (void *data, int count, enum vect_cost_for_stmt kind,
static void initialize_aarch64_code_model (struct gcc_options *);
-/* Enum describing the various ways that the
- aarch64_parse_{arch,tune,cpu,extension} functions can fail.
- This way their callers can choose what kind of error to give. */
-
-enum aarch64_parse_opt_result
-{
- AARCH64_PARSE_OK, /* Parsing was successful. */
- AARCH64_PARSE_MISSING_ARG, /* Missing argument. */
- AARCH64_PARSE_INVALID_FEATURE, /* Invalid feature modifier. */
- AARCH64_PARSE_INVALID_ARG /* Invalid arch, tune, cpu arg. */
-};
-
-/* Parse the architecture extension string STR and update ISA_FLAGS
- with the architecture features turned on or off. Return a
- aarch64_parse_opt_result describing the result. */
-
-static enum aarch64_parse_opt_result
-aarch64_parse_extension (char *str, unsigned long *isa_flags)
-{
- /* The extension string is parsed left to right. */
- const struct aarch64_option_extension *opt = NULL;
-
- /* Flag to say whether we are adding or removing an extension. */
- int adding_ext = -1;
-
- while (str != NULL && *str != 0)
- {
- char *ext;
- size_t len;
-
- str++;
- ext = strchr (str, '+');
-
- if (ext != NULL)
- len = ext - str;
- else
- len = strlen (str);
-
- if (len >= 2 && strncmp (str, "no", 2) == 0)
- {
- adding_ext = 0;
- len -= 2;
- str += 2;
- }
- else if (len > 0)
- adding_ext = 1;
-
- if (len == 0)
- return AARCH64_PARSE_MISSING_ARG;
-
-
- /* Scan over the extensions table trying to find an exact match. */
- for (opt = all_extensions; opt->name != NULL; opt++)
- {
- if (strlen (opt->name) == len && strncmp (opt->name, str, len) == 0)
- {
- /* Add or remove the extension. */
- if (adding_ext)
- *isa_flags |= opt->flags_on;
- else
- *isa_flags &= ~(opt->flags_off);
- break;
- }
- }
-
- if (opt->name == NULL)
- {
- /* Extension not found in list. */
- return AARCH64_PARSE_INVALID_FEATURE;
- }
-
- str = ext;
- };
-
- return AARCH64_PARSE_OK;
-}
-
/* Parse the TO_PARSE string and put the architecture struct that it
selects into RES and the architectural features into ISA_FLAGS.
Return an aarch64_parse_opt_result describing the parse result.
@@ -8550,7 +8473,7 @@ aarch64_option_print (FILE *file, int indent, struct cl_target_option *ptr)
unsigned long isa_flags = ptr->x_aarch64_isa_flags;
const struct processor *arch = aarch64_get_arch (ptr->x_explicit_arch);
std::string extension
- = aarch64_get_extension_string_for_isa_flags (isa_flags);
+ = aarch64_get_extension_string_for_isa_flags (isa_flags, arch->flags);
fprintf (file, "%*sselected tune = %s\n", indent, "", cpu->name);
fprintf (file, "%*sselected arch = %s%s\n", indent, "",
@@ -11213,7 +11136,8 @@ aarch64_declare_function_name (FILE *stream, const char* name,
unsigned long isa_flags = targ_options->x_aarch64_isa_flags;
std::string extension
- = aarch64_get_extension_string_for_isa_flags (isa_flags);
+ = aarch64_get_extension_string_for_isa_flags (isa_flags,
+ this_arch->flags);
/* Only update the assembler .arch string if it is distinct from the last
such string we printed. */
std::string to_print = this_arch->name + extension;
@@ -11253,7 +11177,8 @@ aarch64_start_file (void)
= aarch64_get_arch (default_options->x_explicit_arch);
unsigned long default_isa_flags = default_options->x_aarch64_isa_flags;
std::string extension
- = aarch64_get_extension_string_for_isa_flags (default_isa_flags);
+ = aarch64_get_extension_string_for_isa_flags (default_isa_flags,
+ default_arch->flags);
aarch64_last_printed_arch_string = default_arch->name + extension;
aarch64_last_printed_tune_string = "";
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 317a8a9..8925ec1 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -23,11 +23,12 @@
struct arch_extension
{
const char *ext;
+ unsigned int flag;
const char *feat_string;
};
-#define AARCH64_OPT_EXTENSION(EXT_NAME, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
- { EXT_NAME, FEATURE_STRING },
+#define AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
+ { EXT_NAME, FLAG_CANONICAL, FEATURE_STRING },
static struct arch_extension ext_to_feat_string[] =
{
#include "aarch64-option-extensions.def"
diff --git a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_4.c b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_4.c
index 8eb50aa..49b74d9 100644
--- a/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_4.c
+++ b/gcc/testsuite/gcc.target/aarch64/mgeneral-regs_4.c
@@ -6,4 +6,4 @@ test (void)
return 1;
}
-/* { dg-final { scan-assembler "\.arch.*fp.*simd" } } */
+/* { dg-final { scan-assembler-times "\\.arch armv8-a\n" 1 } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/target_attr_15.c b/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
index f72bec8..2d8c7b9 100644
--- a/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
+++ b/gcc/testsuite/gcc.target/aarch64/target_attr_15.c
@@ -10,4 +10,4 @@ foo (int a)
return a + 1;
}
-/* { dg-final { scan-assembler-times "\\.arch armv8-a\n" 1 } } */
+/* { dg-final { scan-assembler-times "\\.arch armv8-a\\+nofp\\+nosimd\n" 1 } } */
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Patch AArch64 1/3] Enable CRC by default for armv8.1-a
2016-04-06 10:10 [Patch AArch64 0/3] Fix PR70133 James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 2/3] Rework the code to print extension strings (pr70133) James Greenhalgh
@ 2016-04-06 10:10 ` James Greenhalgh
2016-04-07 15:24 ` Christophe Lyon
2016-04-06 10:11 ` [Patch AArch64 3/3] Fix up for pr70133 James Greenhalgh
2016-04-11 9:58 ` [Patch AArch64 0/3] Fix PR70133 Richard Earnshaw (lists)
3 siblings, 1 reply; 9+ messages in thread
From: James Greenhalgh @ 2016-04-06 10:10 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, marcus.shawcroft, richard.earnshaw
[-- Attachment #1: Type: text/plain, Size: 259 bytes --]
Hi,
This change reflects binutils support for CRC, where it is always enabled
for armv8.1-a.
OK?
Thanks,
James
---
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
* config/aarch64/aarch64.h (AARCH64_FL_FOR_ARCH8_1): Also add
AARCH64_FL_CRC.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-AArch64-1-3-Enable-CRC-by-default-for-armv8.1-.patch --]
[-- Type: text/x-patch; name="0001-Patch-AArch64-1-3-Enable-CRC-by-default-for-armv8.1-.patch", Size: 577 bytes --]
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 7750d1c..15d7e40 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -145,7 +145,7 @@ extern unsigned aarch64_architecture_version;
/* Architecture flags that effect instruction selection. */
#define AARCH64_FL_FOR_ARCH8 (AARCH64_FL_FPSIMD)
#define AARCH64_FL_FOR_ARCH8_1 \
- (AARCH64_FL_FOR_ARCH8 | AARCH64_FL_LSE | AARCH64_FL_V8_1)
+ (AARCH64_FL_FOR_ARCH8 | AARCH64_FL_LSE | AARCH64_FL_CRC | AARCH64_FL_V8_1)
/* Macros to test ISA flags. */
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Patch AArch64 0/3] Fix PR70133
@ 2016-04-06 10:10 James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 2/3] Rework the code to print extension strings (pr70133) James Greenhalgh
` (3 more replies)
0 siblings, 4 replies; 9+ messages in thread
From: James Greenhalgh @ 2016-04-06 10:10 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, marcus.shawcroft, richard.earnshaw
[-- Attachment #1: Type: text/plain, Size: 3685 bytes --]
Hi,
This patch set fixes PR70133, which is a bug in the way we handle extension
strings after using -march or -mcpu=native. In investigating this, I found
other bugs in the way we communicate architceture intention between the
compiler and the assembler.
This patch set cleans this up somewhat.
Tested on a Cortex-A57 based board, a Cortex-A57/Cortex-A53 big.LITTLE
based board, a Cortex-A72/Cortex-A53 big.LITTLE based board and an xgene1
based board. I don't have access to the board in the bug report, but I fed
representative data to the detection code to check that worked too.
The patch set goes as follows...
[Patch AArch64 1/3] Enable CRC by default for armv8.1-a
The assmebler will enable CRC by default for -march=armv8.1-a, and we should
follow that expectation in GCC.
[Patch AArch64 2/3] Rework the code to print extension strings (pr70133)
There are a number of bugs that come from the way we enable and disable
extension strings. Rework this code so we always put out a safe and minimal
set of flags for a -march/-mcpu input.
[Patch AArch64 3/3] Fix up for pr70133
Use the infratructure added in 2/3 to fix the PR.
OK for trunk?
Thanks,
James
---
[Patch AArch64 1/3] Enable CRC by default for armv8.1-a
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
* config/aarch64/aarch64.h (AARCH64_FL_FOR_ARCH8_1): Also add
AARCH64_FL_CRC.
[Patch AArch64 2/3] Rework the code to print extension strings (pr70133)
gcc/
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
PR target/70133
* config/aarch64/aarch64-common.c (aarch64_option_extension): Keep
track of a canonical flag name.
(all_extensions): Likewise.
(arch_to_arch_name): Also track extension flags enabled by the arch.
(all_architectures): Likewise.
(aarch64_parse_extension): Move to here.
(aarch64_get_extension_string_for_isa_flags): Take a new argument,
rework.
(aarch64_rewrite_selected_cpu): Update for above change.
* config/aarch64/aarch64-option-extensions.def: Rework the way flags
are handled, such that the single explicit value enabled by an
extension is kept seperate from the implicit values it also enables.
* config/aarch64/aarch64-protos.h (aarch64_parse_opt_result): Move
to here.
(aarch64_parse_extension): New.
* config/aarch64/aarch64.c (aarch64_parse_opt_result): Move from
here to config/aarch64/aarch64-protos.h.
(aarch64_parse_extension): Move from here to
common/config/aarch64/aarch64-common.c.
(aarch64_option_print): Update.
(aarch64_declare_function_name): Likewise.
(aarch64_start_file): Likewise.
* config/aarch64/driver-aarch64.c (arch_extension): Keep track of
the canonical flag for extensions.
* config.gcc (aarch64*-*-*): Extend regex for capturing extension
flags.
gcc/testsuite/
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
PR target/70133
* gcc.target/aarch64/mgeneral-regs_4.c: Fix expected output.
* gcc.target/aarch64/target_attr_15.c: Likewise.
[Patch AArch64 3/3] Fix up for pr70133
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
PR target/70133
* config/aarch64/driver-aarch64.c
(aarch64_get_extension_string_for_isa_flags): New.
(arch_extension): Rename to...
(aarch64_arch_extension): ...This.
(ext_to_feat_string): Rename to...
(aarch64_extensions): ...This.
(aarch64_core_data): Keep track of architecture extension flags.
(cpu_data): Rename to...
(aarch64_cpu_data): ...This.
(aarch64_arch_driver_info): Keep track of architecture extension
flags.
(get_arch_name_from_id): Rename to...
(get_arch_from_id): ...This, change return type.
(host_detect_local_cpu): Update and reformat for renames, handle
extensions through common infrastructure.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Patch AArch64 3/3] Fix up for pr70133
2016-04-06 10:10 [Patch AArch64 0/3] Fix PR70133 James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 2/3] Rework the code to print extension strings (pr70133) James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 1/3] Enable CRC by default for armv8.1-a James Greenhalgh
@ 2016-04-06 10:11 ` James Greenhalgh
2016-04-07 12:23 ` Kyrill Tkachov
2016-04-11 9:22 ` James Greenhalgh
2016-04-11 9:58 ` [Patch AArch64 0/3] Fix PR70133 Richard Earnshaw (lists)
3 siblings, 2 replies; 9+ messages in thread
From: James Greenhalgh @ 2016-04-06 10:11 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, marcus.shawcroft, richard.earnshaw
[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]
Hi,
Having updated the way we parse and output extension strings, now we just
need to wire up the native detection to use these new features.
In doing some cleanup and rename I ended up fixing 8-spaces to tabs in
about half the file. I've done the rest while I'm here to save us from
some a mixed-style file.
Bootstrapped on aarch64-none-linux-gnu, then tested with defaults and
an explicit -march=native passed (on a system detected as
cortex-a57+crypto, and again on a system detected as
cortex-a72.cortex-a53+crypto). I also set up a dummy /proc/cpuinfo and
used that to manually check the input data in pr70133.
OK?
Thanks,
James
---
2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
PR target/70133
* config/aarch64/driver-aarch64.c
(aarch64_get_extension_string_for_isa_flags): New.
(arch_extension): Rename to...
(aarch64_arch_extension): ...This.
(ext_to_feat_string): Rename to...
(aarch64_extensions): ...This.
(aarch64_core_data): Keep track of architecture extension flags.
(cpu_data): Rename to...
(aarch64_cpu_data): ...This.
(aarch64_arch_driver_info): Keep track of architecture extension
flags.
(get_arch_name_from_id): Rename to...
(get_arch_from_id): ...This, change return type.
(host_detect_local_cpu): Update and reformat for renames, handle
extensions through common infrastructure.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0003-Patch-AArch64-3-3-Fix-up-for-pr70133.patch --]
[-- Type: text/x-patch; name="0003-Patch-AArch64-3-3-Fix-up-for-pr70133.patch", Size: 9440 bytes --]
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 8925ec1..ce771ec 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -18,9 +18,16 @@
<http://www.gnu.org/licenses/>. */
#include "config.h"
+#define INCLUDE_STRING
#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
-struct arch_extension
+/* Defined in common/config/aarch64/aarch64-common.c. */
+std::string aarch64_get_extension_string_for_isa_flags (unsigned long,
+ unsigned long);
+
+struct aarch64_arch_extension
{
const char *ext;
unsigned int flag;
@@ -29,7 +36,7 @@ struct arch_extension
#define AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
{ EXT_NAME, FLAG_CANONICAL, FEATURE_STRING },
-static struct arch_extension ext_to_feat_string[] =
+static struct aarch64_arch_extension aarch64_extensions[] =
{
#include "aarch64-option-extensions.def"
};
@@ -42,15 +49,16 @@ struct aarch64_core_data
const char* arch;
const char* implementer_id;
const char* part_no;
+ const unsigned long flags;
};
#define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
- { CORE_NAME, #ARCH, IMP, PART },
+ { CORE_NAME, #ARCH, IMP, PART, FLAGS },
-static struct aarch64_core_data cpu_data [] =
+static struct aarch64_core_data aarch64_cpu_data[] =
{
#include "aarch64-cores.def"
- { NULL, NULL, NULL, NULL }
+ { NULL, NULL, NULL, NULL, 0 }
};
#undef AARCH64_CORE
@@ -59,37 +67,37 @@ struct aarch64_arch_driver_info
{
const char* id;
const char* name;
+ const unsigned long flags;
};
#define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH_REV, FLAGS) \
- { #ARCH_IDENT, NAME },
+ { #ARCH_IDENT, NAME, FLAGS },
-static struct aarch64_arch_driver_info aarch64_arches [] =
+static struct aarch64_arch_driver_info aarch64_arches[] =
{
#include "aarch64-arches.def"
- {NULL, NULL}
+ {NULL, NULL, 0}
};
#undef AARCH64_ARCH
-/* Return the full architecture name string corresponding to the
- identifier ID. */
+/* Return an aarch64_arch_driver_info for the architecture described
+ by ID, or NULL if ID describes something we don't know about. */
-static const char*
-get_arch_name_from_id (const char* id)
+static struct aarch64_arch_driver_info*
+get_arch_from_id (const char* id)
{
unsigned int i = 0;
for (i = 0; aarch64_arches[i].id != NULL; i++)
{
if (strcmp (id, aarch64_arches[i].id) == 0)
- return aarch64_arches[i].name;
+ return &aarch64_arches[i];
}
return NULL;
}
-
/* Check wether the string CORE contains the same CPU part numbers
as BL_STRING. For example CORE="{0xd03, 0xd07}" and BL_STRING="0xd07.0xd03"
should return true. */
@@ -98,7 +106,7 @@ static bool
valid_bL_string_p (const char** core, const char* bL_string)
{
return strstr (bL_string, core[0]) != NULL
- && strstr (bL_string, core[1]) != NULL;
+ && strstr (bL_string, core[1]) != NULL;
}
/* Return true iff ARR contains STR in one of its two elements. */
@@ -142,7 +150,7 @@ host_detect_local_cpu (int argc, const char **argv)
{
const char *arch_id = NULL;
const char *res = NULL;
- static const int num_exts = ARRAY_SIZE (ext_to_feat_string);
+ static const int num_exts = ARRAY_SIZE (aarch64_extensions);
char buf[128];
FILE *f = NULL;
bool arch = false;
@@ -156,6 +164,8 @@ host_detect_local_cpu (int argc, const char **argv)
unsigned int n_imps = 0;
bool processed_exts = false;
const char *ext_string = "";
+ unsigned long extension_flags = 0;
+ unsigned long default_flags = 0;
gcc_assert (argc);
@@ -184,60 +194,71 @@ host_detect_local_cpu (int argc, const char **argv)
{
if (strstr (buf, "implementer") != NULL)
{
- for (i = 0; cpu_data[i].name != NULL; i++)
- if (strstr (buf, cpu_data[i].implementer_id) != NULL
- && !contains_string_p (imps, cpu_data[i].implementer_id))
+ for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
+ if (strstr (buf, aarch64_cpu_data[i].implementer_id) != NULL
+ && !contains_string_p (imps,
+ aarch64_cpu_data[i].implementer_id))
{
- if (n_imps == 2)
- goto not_found;
+ if (n_imps == 2)
+ goto not_found;
- imps[n_imps++] = cpu_data[i].implementer_id;
+ imps[n_imps++] = aarch64_cpu_data[i].implementer_id;
- break;
+ break;
}
- continue;
+ continue;
}
if (strstr (buf, "part") != NULL)
{
- for (i = 0; cpu_data[i].name != NULL; i++)
- if (strstr (buf, cpu_data[i].part_no) != NULL
- && !contains_string_p (cores, cpu_data[i].part_no))
+ for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
+ if (strstr (buf, aarch64_cpu_data[i].part_no) != NULL
+ && !contains_string_p (cores, aarch64_cpu_data[i].part_no))
{
- if (n_cores == 2)
- goto not_found;
+ if (n_cores == 2)
+ goto not_found;
- cores[n_cores++] = cpu_data[i].part_no;
- core_idx = i;
- arch_id = cpu_data[i].arch;
- break;
+ cores[n_cores++] = aarch64_cpu_data[i].part_no;
+ core_idx = i;
+ arch_id = aarch64_cpu_data[i].arch;
+ break;
}
- continue;
- }
+ continue;
+ }
if (!tune && !processed_exts && strstr (buf, "Features") != NULL)
- {
- for (i = 0; i < num_exts; i++)
- {
- bool enabled = true;
- char *p = NULL;
- char *feat_string = concat (ext_to_feat_string[i].feat_string, NULL);
-
- p = strtok (feat_string, " ");
-
- while (p != NULL)
- {
- if (strstr (buf, p) == NULL)
- {
- enabled = false;
- break;
- }
- p = strtok (NULL, " ");
- }
- ext_string = concat (ext_string, "+", enabled ? "" : "no",
- ext_to_feat_string[i].ext, NULL);
- }
- processed_exts = true;
- }
+ {
+ for (i = 0; i < num_exts; i++)
+ {
+ char *p = NULL;
+ char *feat_string
+ = concat (aarch64_extensions[i].feat_string, NULL);
+ bool enabled = true;
+
+ /* This may be a multi-token feature string. We need
+ to match all parts, which could be in any order.
+ If this isn't a multi-token feature string, strtok is
+ just going to return a pointer to feat_string. */
+ p = strtok (feat_string, " ");
+ while (p != NULL)
+ {
+ if (strstr (buf, p) == NULL)
+ {
+ /* Failed to match this token. Turn off the
+ features we'd otherwise enable. */
+ enabled = false;
+ break;
+ }
+ p = strtok (NULL, " ");
+ }
+
+ if (enabled)
+ extension_flags |= aarch64_extensions[i].flag;
+ else
+ extension_flags &= ~(aarch64_extensions[i].flag);
+ }
+
+ processed_exts = true;
+ }
}
fclose (f);
@@ -252,44 +273,56 @@ host_detect_local_cpu (int argc, const char **argv)
if (arch)
{
- const char* arch_name = get_arch_name_from_id (arch_id);
+ struct aarch64_arch_driver_info* arch_info = get_arch_from_id (arch_id);
/* We got some arch indentifier that's not in aarch64-arches.def? */
- if (!arch_name)
- goto not_found;
+ if (!arch_info)
+ goto not_found;
- res = concat ("-march=", arch_name, NULL);
+ res = concat ("-march=", arch_info->name, NULL);
+ default_flags = arch_info->flags;
}
/* We have big.LITTLE. */
else if (n_cores == 2)
{
- for (i = 0; cpu_data[i].name != NULL; i++)
- {
- if (strchr (cpu_data[i].part_no, '.') != NULL
- && strncmp (cpu_data[i].implementer_id, imps[0], strlen (imps[0]) - 1) == 0
- && valid_bL_string_p (cores, cpu_data[i].part_no))
- {
- res = concat ("-m", cpu ? "cpu" : "tune", "=", cpu_data[i].name, NULL);
- break;
- }
- }
+ for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
+ {
+ if (strchr (aarch64_cpu_data[i].part_no, '.') != NULL
+ && strncmp (aarch64_cpu_data[i].implementer_id,
+ imps[0],
+ strlen (imps[0]) - 1) == 0
+ && valid_bL_string_p (cores, aarch64_cpu_data[i].part_no))
+ {
+ res = concat ("-m",
+ cpu ? "cpu" : "tune", "=",
+ aarch64_cpu_data[i].name,
+ NULL);
+ default_flags = aarch64_cpu_data[i].flags;
+ break;
+ }
+ }
if (!res)
- goto not_found;
+ goto not_found;
}
/* The simple, non-big.LITTLE case. */
else
{
- if (strncmp (cpu_data[core_idx].implementer_id, imps[0],
- strlen (imps[0]) - 1) != 0)
- goto not_found;
+ if (strncmp (aarch64_cpu_data[core_idx].implementer_id, imps[0],
+ strlen (imps[0]) - 1) != 0)
+ goto not_found;
res = concat ("-m", cpu ? "cpu" : "tune", "=",
- cpu_data[core_idx].name, NULL);
+ aarch64_cpu_data[core_idx].name, NULL);
+ default_flags = aarch64_cpu_data[core_idx].flags;
}
if (tune)
return res;
+ ext_string
+ = aarch64_get_extension_string_for_isa_flags (extension_flags,
+ default_flags).c_str ();
+
res = concat (res, ext_string, NULL);
return res;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch AArch64 3/3] Fix up for pr70133
2016-04-06 10:11 ` [Patch AArch64 3/3] Fix up for pr70133 James Greenhalgh
@ 2016-04-07 12:23 ` Kyrill Tkachov
2016-04-11 9:22 ` James Greenhalgh
1 sibling, 0 replies; 9+ messages in thread
From: Kyrill Tkachov @ 2016-04-07 12:23 UTC (permalink / raw)
To: James Greenhalgh, gcc-patches; +Cc: marcus.shawcroft, richard.earnshaw
Hi all,
On 06/04/16 11:10, James Greenhalgh wrote:
> Hi,
>
> Having updated the way we parse and output extension strings, now we just
> need to wire up the native detection to use these new features.
>
> In doing some cleanup and rename I ended up fixing 8-spaces to tabs in
> about half the file. I've done the rest while I'm here to save us from
> some a mixed-style file.
>
> Bootstrapped on aarch64-none-linux-gnu, then tested with defaults and
> an explicit -march=native passed (on a system detected as
> cortex-a57+crypto, and again on a system detected as
> cortex-a72.cortex-a53+crypto). I also set up a dummy /proc/cpuinfo and
> used that to manually check the input data in pr70133.
>
> OK?
This looks good to me (but I can't approve).
Thanks,
Kyrill
> Thanks,
> James
>
> ---
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> PR target/70133
>
> * config/aarch64/driver-aarch64.c
> (aarch64_get_extension_string_for_isa_flags): New.
> (arch_extension): Rename to...
> (aarch64_arch_extension): ...This.
> (ext_to_feat_string): Rename to...
> (aarch64_extensions): ...This.
> (aarch64_core_data): Keep track of architecture extension flags.
> (cpu_data): Rename to...
> (aarch64_cpu_data): ...This.
> (aarch64_arch_driver_info): Keep track of architecture extension
> flags.
> (get_arch_name_from_id): Rename to...
> (get_arch_from_id): ...This, change return type.
> (host_detect_local_cpu): Update and reformat for renames, handle
> extensions through common infrastructure.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch AArch64 1/3] Enable CRC by default for armv8.1-a
2016-04-06 10:10 ` [Patch AArch64 1/3] Enable CRC by default for armv8.1-a James Greenhalgh
@ 2016-04-07 15:24 ` Christophe Lyon
2016-04-07 16:52 ` James Greenhalgh
0 siblings, 1 reply; 9+ messages in thread
From: Christophe Lyon @ 2016-04-07 15:24 UTC (permalink / raw)
To: James Greenhalgh; +Cc: gcc-patches, nd, Marcus Shawcroft, Richard Earnshaw
On 6 April 2016 at 12:10, James Greenhalgh <james.greenhalgh@arm.com> wrote:
>
> Hi,
>
> This change reflects binutils support for CRC, where it is always enabled
> for armv8.1-a.
>
Does v8.1 always enable CRC?
If not, then don't you want to change the binutils default instead?
Christophe.
> OK?
>
> Thanks,
> James
>
> ---
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> * config/aarch64/aarch64.h (AARCH64_FL_FOR_ARCH8_1): Also add
> AARCH64_FL_CRC.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch AArch64 1/3] Enable CRC by default for armv8.1-a
2016-04-07 15:24 ` Christophe Lyon
@ 2016-04-07 16:52 ` James Greenhalgh
0 siblings, 0 replies; 9+ messages in thread
From: James Greenhalgh @ 2016-04-07 16:52 UTC (permalink / raw)
To: Christophe Lyon; +Cc: gcc-patches, nd, Marcus Shawcroft, Richard Earnshaw
On Thu, Apr 07, 2016 at 05:23:59PM +0200, Christophe Lyon wrote:
> On 6 April 2016 at 12:10, James Greenhalgh <james.greenhalgh@arm.com> wrote:
> >
> > Hi,
> >
> > This change reflects binutils support for CRC, where it is always enabled
> > for armv8.1-a.
> >
>
> Does v8.1 always enable CRC?
Yes. -march=armv8.1-a should always enable CRC. Unfortunately some
binutils versions do not honour this, which is why in the next patch we
must always put +crc out.
> If not, then don't you want to change the binutils default instead?
No, this patch is the correct thing to do - regadless of what binutils does,
GCC should enable access to the CRC intrinsics with -march=armv8.1-a, so we
want this patch.
Thanks,
James
> > ---
> > 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
> >
> > * config/aarch64/aarch64.h (AARCH64_FL_FOR_ARCH8_1): Also add
> > AARCH64_FL_CRC.
> >
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch AArch64 3/3] Fix up for pr70133
2016-04-06 10:11 ` [Patch AArch64 3/3] Fix up for pr70133 James Greenhalgh
2016-04-07 12:23 ` Kyrill Tkachov
@ 2016-04-11 9:22 ` James Greenhalgh
1 sibling, 0 replies; 9+ messages in thread
From: James Greenhalgh @ 2016-04-11 9:22 UTC (permalink / raw)
To: gcc-patches; +Cc: nd, richard.earnshaw, kyrtka01, marcus.shawcroft
[-- Attachment #1: Type: text/plain, Size: 1372 bytes --]
> Hi,
>
> Having updated the way we parse and output extension strings, now we just
> need to wire up the native detection to use these new features.
>
> In doing some cleanup and rename I ended up fixing 8-spaces to tabs in
> about half the file. I've done the rest while I'm here to save us from
> some a mixed-style file.
Richard Earnshaw pointed out off-list that it was perhaps a little
anti-social to ask a reviewer to wade through all the tabs-to-spaces changes
to find the real content of this patch. I can't disagree with that.
Here's the diff regenerated with '-w' to skip the whitespace changes.
OK?
Thanks,
James
> ---
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> PR target/70133
>
> * config/aarch64/driver-aarch64.c
> (aarch64_get_extension_string_for_isa_flags): New.
> (arch_extension): Rename to...
> (aarch64_arch_extension): ...This.
> (ext_to_feat_string): Rename to...
> (aarch64_extensions): ...This.
> (aarch64_core_data): Keep track of architecture extension flags.
> (cpu_data): Rename to...
> (aarch64_cpu_data): ...This.
> (aarch64_arch_driver_info): Keep track of architecture extension
> flags.
> (get_arch_name_from_id): Rename to...
> (get_arch_from_id): ...This, change return type.
> (host_detect_local_cpu): Update and reformat for renames, handle
> extensions through common infrastructure.
>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Patch-AArch64-3-3-Fix-up-for-pr70133.patch --]
[-- Type: text/x-patch; name="0001-Patch-AArch64-3-3-Fix-up-for-pr70133.patch", Size: 8197 bytes --]
diff --git a/gcc/config/aarch64/driver-aarch64.c b/gcc/config/aarch64/driver-aarch64.c
index 8925ec1..ce771ec 100644
--- a/gcc/config/aarch64/driver-aarch64.c
+++ b/gcc/config/aarch64/driver-aarch64.c
@@ -18,9 +18,16 @@
<http://www.gnu.org/licenses/>. */
#include "config.h"
+#define INCLUDE_STRING
#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
-struct arch_extension
+/* Defined in common/config/aarch64/aarch64-common.c. */
+std::string aarch64_get_extension_string_for_isa_flags (unsigned long,
+ unsigned long);
+
+struct aarch64_arch_extension
{
const char *ext;
unsigned int flag;
@@ -29,7 +36,7 @@ struct arch_extension
#define AARCH64_OPT_EXTENSION(EXT_NAME, FLAG_CANONICAL, FLAGS_ON, FLAGS_OFF, FEATURE_STRING) \
{ EXT_NAME, FLAG_CANONICAL, FEATURE_STRING },
-static struct arch_extension ext_to_feat_string[] =
+static struct aarch64_arch_extension aarch64_extensions[] =
{
#include "aarch64-option-extensions.def"
};
@@ -42,15 +49,16 @@ struct aarch64_core_data
const char* arch;
const char* implementer_id;
const char* part_no;
+ const unsigned long flags;
};
#define AARCH64_CORE(CORE_NAME, CORE_IDENT, SCHED, ARCH, FLAGS, COSTS, IMP, PART) \
- { CORE_NAME, #ARCH, IMP, PART },
+ { CORE_NAME, #ARCH, IMP, PART, FLAGS },
-static struct aarch64_core_data cpu_data [] =
+static struct aarch64_core_data aarch64_cpu_data[] =
{
#include "aarch64-cores.def"
- { NULL, NULL, NULL, NULL }
+ { NULL, NULL, NULL, NULL, 0 }
};
#undef AARCH64_CORE
@@ -59,37 +67,37 @@ struct aarch64_arch_driver_info
{
const char* id;
const char* name;
+ const unsigned long flags;
};
#define AARCH64_ARCH(NAME, CORE, ARCH_IDENT, ARCH_REV, FLAGS) \
- { #ARCH_IDENT, NAME },
+ { #ARCH_IDENT, NAME, FLAGS },
static struct aarch64_arch_driver_info aarch64_arches[] =
{
#include "aarch64-arches.def"
- {NULL, NULL}
+ {NULL, NULL, 0}
};
#undef AARCH64_ARCH
-/* Return the full architecture name string corresponding to the
- identifier ID. */
+/* Return an aarch64_arch_driver_info for the architecture described
+ by ID, or NULL if ID describes something we don't know about. */
-static const char*
-get_arch_name_from_id (const char* id)
+static struct aarch64_arch_driver_info*
+get_arch_from_id (const char* id)
{
unsigned int i = 0;
for (i = 0; aarch64_arches[i].id != NULL; i++)
{
if (strcmp (id, aarch64_arches[i].id) == 0)
- return aarch64_arches[i].name;
+ return &aarch64_arches[i];
}
return NULL;
}
-
/* Check wether the string CORE contains the same CPU part numbers
as BL_STRING. For example CORE="{0xd03, 0xd07}" and BL_STRING="0xd07.0xd03"
should return true. */
@@ -142,7 +150,7 @@ host_detect_local_cpu (int argc, const char **argv)
{
const char *arch_id = NULL;
const char *res = NULL;
- static const int num_exts = ARRAY_SIZE (ext_to_feat_string);
+ static const int num_exts = ARRAY_SIZE (aarch64_extensions);
char buf[128];
FILE *f = NULL;
bool arch = false;
@@ -156,6 +164,8 @@ host_detect_local_cpu (int argc, const char **argv)
unsigned int n_imps = 0;
bool processed_exts = false;
const char *ext_string = "";
+ unsigned long extension_flags = 0;
+ unsigned long default_flags = 0;
gcc_assert (argc);
@@ -184,14 +194,15 @@ host_detect_local_cpu (int argc, const char **argv)
{
if (strstr (buf, "implementer") != NULL)
{
- for (i = 0; cpu_data[i].name != NULL; i++)
- if (strstr (buf, cpu_data[i].implementer_id) != NULL
- && !contains_string_p (imps, cpu_data[i].implementer_id))
+ for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
+ if (strstr (buf, aarch64_cpu_data[i].implementer_id) != NULL
+ && !contains_string_p (imps,
+ aarch64_cpu_data[i].implementer_id))
{
if (n_imps == 2)
goto not_found;
- imps[n_imps++] = cpu_data[i].implementer_id;
+ imps[n_imps++] = aarch64_cpu_data[i].implementer_id;
break;
}
@@ -200,16 +211,16 @@ host_detect_local_cpu (int argc, const char **argv)
if (strstr (buf, "part") != NULL)
{
- for (i = 0; cpu_data[i].name != NULL; i++)
- if (strstr (buf, cpu_data[i].part_no) != NULL
- && !contains_string_p (cores, cpu_data[i].part_no))
+ for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
+ if (strstr (buf, aarch64_cpu_data[i].part_no) != NULL
+ && !contains_string_p (cores, aarch64_cpu_data[i].part_no))
{
if (n_cores == 2)
goto not_found;
- cores[n_cores++] = cpu_data[i].part_no;
+ cores[n_cores++] = aarch64_cpu_data[i].part_no;
core_idx = i;
- arch_id = cpu_data[i].arch;
+ arch_id = aarch64_cpu_data[i].arch;
break;
}
continue;
@@ -218,24 +229,34 @@ host_detect_local_cpu (int argc, const char **argv)
{
for (i = 0; i < num_exts; i++)
{
- bool enabled = true;
char *p = NULL;
- char *feat_string = concat (ext_to_feat_string[i].feat_string, NULL);
+ char *feat_string
+ = concat (aarch64_extensions[i].feat_string, NULL);
+ bool enabled = true;
+ /* This may be a multi-token feature string. We need
+ to match all parts, which could be in any order.
+ If this isn't a multi-token feature string, strtok is
+ just going to return a pointer to feat_string. */
p = strtok (feat_string, " ");
-
while (p != NULL)
{
if (strstr (buf, p) == NULL)
{
+ /* Failed to match this token. Turn off the
+ features we'd otherwise enable. */
enabled = false;
break;
}
p = strtok (NULL, " ");
}
- ext_string = concat (ext_string, "+", enabled ? "" : "no",
- ext_to_feat_string[i].ext, NULL);
+
+ if (enabled)
+ extension_flags |= aarch64_extensions[i].flag;
+ else
+ extension_flags &= ~(aarch64_extensions[i].flag);
}
+
processed_exts = true;
}
}
@@ -252,24 +273,31 @@ host_detect_local_cpu (int argc, const char **argv)
if (arch)
{
- const char* arch_name = get_arch_name_from_id (arch_id);
+ struct aarch64_arch_driver_info* arch_info = get_arch_from_id (arch_id);
/* We got some arch indentifier that's not in aarch64-arches.def? */
- if (!arch_name)
+ if (!arch_info)
goto not_found;
- res = concat ("-march=", arch_name, NULL);
+ res = concat ("-march=", arch_info->name, NULL);
+ default_flags = arch_info->flags;
}
/* We have big.LITTLE. */
else if (n_cores == 2)
{
- for (i = 0; cpu_data[i].name != NULL; i++)
+ for (i = 0; aarch64_cpu_data[i].name != NULL; i++)
{
- if (strchr (cpu_data[i].part_no, '.') != NULL
- && strncmp (cpu_data[i].implementer_id, imps[0], strlen (imps[0]) - 1) == 0
- && valid_bL_string_p (cores, cpu_data[i].part_no))
+ if (strchr (aarch64_cpu_data[i].part_no, '.') != NULL
+ && strncmp (aarch64_cpu_data[i].implementer_id,
+ imps[0],
+ strlen (imps[0]) - 1) == 0
+ && valid_bL_string_p (cores, aarch64_cpu_data[i].part_no))
{
- res = concat ("-m", cpu ? "cpu" : "tune", "=", cpu_data[i].name, NULL);
+ res = concat ("-m",
+ cpu ? "cpu" : "tune", "=",
+ aarch64_cpu_data[i].name,
+ NULL);
+ default_flags = aarch64_cpu_data[i].flags;
break;
}
}
@@ -279,17 +307,22 @@ host_detect_local_cpu (int argc, const char **argv)
/* The simple, non-big.LITTLE case. */
else
{
- if (strncmp (cpu_data[core_idx].implementer_id, imps[0],
+ if (strncmp (aarch64_cpu_data[core_idx].implementer_id, imps[0],
strlen (imps[0]) - 1) != 0)
goto not_found;
res = concat ("-m", cpu ? "cpu" : "tune", "=",
- cpu_data[core_idx].name, NULL);
+ aarch64_cpu_data[core_idx].name, NULL);
+ default_flags = aarch64_cpu_data[core_idx].flags;
}
if (tune)
return res;
+ ext_string
+ = aarch64_get_extension_string_for_isa_flags (extension_flags,
+ default_flags).c_str ();
+
res = concat (res, ext_string, NULL);
return res;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Patch AArch64 0/3] Fix PR70133
2016-04-06 10:10 [Patch AArch64 0/3] Fix PR70133 James Greenhalgh
` (2 preceding siblings ...)
2016-04-06 10:11 ` [Patch AArch64 3/3] Fix up for pr70133 James Greenhalgh
@ 2016-04-11 9:58 ` Richard Earnshaw (lists)
3 siblings, 0 replies; 9+ messages in thread
From: Richard Earnshaw (lists) @ 2016-04-11 9:58 UTC (permalink / raw)
To: James Greenhalgh, gcc-patches; +Cc: nd, marcus.shawcroft
On 06/04/16 11:10, James Greenhalgh wrote:
> Hi,
>
> This patch set fixes PR70133, which is a bug in the way we handle extension
> strings after using -march or -mcpu=native. In investigating this, I found
> other bugs in the way we communicate architceture intention between the
> compiler and the assembler.
>
> This patch set cleans this up somewhat.
>
> Tested on a Cortex-A57 based board, a Cortex-A57/Cortex-A53 big.LITTLE
> based board, a Cortex-A72/Cortex-A53 big.LITTLE based board and an xgene1
> based board. I don't have access to the board in the bug report, but I fed
> representative data to the detection code to check that worked too.
>
> The patch set goes as follows...
>
> [Patch AArch64 1/3] Enable CRC by default for armv8.1-a
>
> The assmebler will enable CRC by default for -march=armv8.1-a, and we should
> follow that expectation in GCC.
>
> [Patch AArch64 2/3] Rework the code to print extension strings (pr70133)
>
> There are a number of bugs that come from the way we enable and disable
> extension strings. Rework this code so we always put out a safe and minimal
> set of flags for a -march/-mcpu input.
>
> [Patch AArch64 3/3] Fix up for pr70133
>
> Use the infratructure added in 2/3 to fix the PR.
>
> OK for trunk?
>
OK for all of these.
R.
> Thanks,
> James
>
> ---
> [Patch AArch64 1/3] Enable CRC by default for armv8.1-a
>
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> * config/aarch64/aarch64.h (AARCH64_FL_FOR_ARCH8_1): Also add
> AARCH64_FL_CRC.
>
>
> [Patch AArch64 2/3] Rework the code to print extension strings (pr70133)
>
> gcc/
>
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> PR target/70133
> * config/aarch64/aarch64-common.c (aarch64_option_extension): Keep
> track of a canonical flag name.
> (all_extensions): Likewise.
> (arch_to_arch_name): Also track extension flags enabled by the arch.
> (all_architectures): Likewise.
> (aarch64_parse_extension): Move to here.
> (aarch64_get_extension_string_for_isa_flags): Take a new argument,
> rework.
> (aarch64_rewrite_selected_cpu): Update for above change.
> * config/aarch64/aarch64-option-extensions.def: Rework the way flags
> are handled, such that the single explicit value enabled by an
> extension is kept seperate from the implicit values it also enables.
> * config/aarch64/aarch64-protos.h (aarch64_parse_opt_result): Move
> to here.
> (aarch64_parse_extension): New.
> * config/aarch64/aarch64.c (aarch64_parse_opt_result): Move from
> here to config/aarch64/aarch64-protos.h.
> (aarch64_parse_extension): Move from here to
> common/config/aarch64/aarch64-common.c.
> (aarch64_option_print): Update.
> (aarch64_declare_function_name): Likewise.
> (aarch64_start_file): Likewise.
> * config/aarch64/driver-aarch64.c (arch_extension): Keep track of
> the canonical flag for extensions.
> * config.gcc (aarch64*-*-*): Extend regex for capturing extension
> flags.
>
> gcc/testsuite/
>
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> PR target/70133
> * gcc.target/aarch64/mgeneral-regs_4.c: Fix expected output.
> * gcc.target/aarch64/target_attr_15.c: Likewise.
>
> [Patch AArch64 3/3] Fix up for pr70133
>
> 2016-04-06 James Greenhalgh <james.greenhalgh@arm.com>
>
> PR target/70133
>
> * config/aarch64/driver-aarch64.c
> (aarch64_get_extension_string_for_isa_flags): New.
> (arch_extension): Rename to...
> (aarch64_arch_extension): ...This.
> (ext_to_feat_string): Rename to...
> (aarch64_extensions): ...This.
> (aarch64_core_data): Keep track of architecture extension flags.
> (cpu_data): Rename to...
> (aarch64_cpu_data): ...This.
> (aarch64_arch_driver_info): Keep track of architecture extension
> flags.
> (get_arch_name_from_id): Rename to...
> (get_arch_from_id): ...This, change return type.
> (host_detect_local_cpu): Update and reformat for renames, handle
> extensions through common infrastructure.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-04-11 9:58 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-06 10:10 [Patch AArch64 0/3] Fix PR70133 James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 2/3] Rework the code to print extension strings (pr70133) James Greenhalgh
2016-04-06 10:10 ` [Patch AArch64 1/3] Enable CRC by default for armv8.1-a James Greenhalgh
2016-04-07 15:24 ` Christophe Lyon
2016-04-07 16:52 ` James Greenhalgh
2016-04-06 10:11 ` [Patch AArch64 3/3] Fix up for pr70133 James Greenhalgh
2016-04-07 12:23 ` Kyrill Tkachov
2016-04-11 9:22 ` James Greenhalgh
2016-04-11 9:58 ` [Patch AArch64 0/3] Fix PR70133 Richard Earnshaw (lists)
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).