* Add .def file for public target instructions
@ 2015-06-23 18:42 Richard Sandiford
2015-06-24 6:36 ` Jeff Law
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Richard Sandiford @ 2015-06-23 18:42 UTC (permalink / raw)
To: gcc-patches; +Cc: Mikhail Maltsev
[A fair bit later than promised, sorry...]
Mikhail posted a patch to make genflags generate the default HAVE_foo
and gen_foo definitions that have recently been added to defaults.h:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
I agree it'd be a good idea to generate this kind of thing automatically,
but I think we should take the opportunity to move the interface to the
target structure. I.e.:
HAVE_foo -> targetm.have_foo ()
gen_foo -> targetm.gen_foo ()
This should move us closer to the pipedream goal of supporting multiple
targets at once. It should also mean that only the target code depends
on insn-flags.h.
The patch just moves return and simple_return as an example. I have more
locally (in order to test other code paths), but they're just an obvious
extension of this one.
The patch relies on the hashing changes in:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
and on this trivial patch:
https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
It seems a bit heavyweight when you just look at these two instructions,
but I think it'll be a saving in the end.
Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
via config-list.mk. OK to install?
Thanks,
Richard
gcc/
* Makefile.in (TARGET_DEF): Add target-insns.def.
(.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
(build/gentarget-def.o): New rule.
(genprogrtl): Add target-def.
* target-insns.def, gentarget-def.c: New files.
* target.def: Add targetm.have_* and targetm.gen_* hooks,
based on the contents of target-insns.def.
* defaults.h (HAVE_simple_return, gen_simple_return): Delete.
(HAVE_return, gen_return): Delete.
* target-def.h: Include insn-target-def.h.
* cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
instead of direct calls. Rely on them to do the appropriate assertions.
* function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
(convert_jumps_to_returns): Use targetm interface instead of
direct calls.
(thread_prologue_and_epilogue_insns): Likewise.
* reorg.c (find_end_label, dbr_schedule): Likewise.
* shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
* shrink-wrap.c (convert_to_simple_return): Likewise.
(try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in 2015-06-22 14:03:10.985503368 +0100
+++ gcc/Makefile.in 2015-06-22 14:04:26.689971484 +0100
@@ -866,7 +866,7 @@ DUMPFILE_H = $(srcdir)/../libcpp/include
VEC_H = vec.h statistics.h $(GGC_H)
HASH_TABLE_H = $(HASHTAB_H) hash-table.h
EXCEPT_H = except.h $(HASHTAB_H)
-TARGET_DEF = target.def target-hooks-macros.h
+TARGET_DEF = target.def target-hooks-macros.h target-insns.def
C_TARGET_DEF = c-family/c-target.def target-hooks-macros.h
COMMON_TARGET_DEF = common/common-target.def target-hooks-macros.h
TARGET_H = $(TM_H) target.h $(TARGET_DEF) insn-modes.h insn-codes.h
@@ -2078,7 +2078,8 @@ $(common_out_object_file): $(common_out_
.PRECIOUS: insn-config.h insn-flags.h insn-codes.h insn-constants.h \
insn-emit.c insn-recog.c insn-extract.c insn-output.c insn-peep.c \
insn-attr.h insn-attr-common.h insn-attrtab.c insn-dfatab.c \
- insn-latencytab.c insn-preds.c gimple-match.c generic-match.c
+ insn-latencytab.c insn-preds.c gimple-match.c generic-match.c \
+ insn-target-def.h
# Dependencies for the md file. The first time through, we just assume
# the md file itself and the generated dependency file (in order to get
@@ -2099,7 +2100,7 @@ s-mddeps: $(md_file) $(MD_INCLUDES) buil
# the target file.
simple_rtl_generated_h = insn-attr.h insn-attr-common.h insn-codes.h \
- insn-config.h insn-flags.h
+ insn-config.h insn-flags.h insn-target-def.h
simple_rtl_generated_c = insn-automata.c insn-emit.c \
insn-extract.c insn-output.c \
@@ -2498,6 +2499,9 @@ build/genextract.o : genextract.c $(RTL_
$(SYSTEM_H) coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h
build/genflags.o : genflags.c $(RTL_BASE_H) $(OBSTACK_H) $(BCONFIG_H) \
$(SYSTEM_H) coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h
+build/gentarget-def.o : gentarget-def.c $(BCONFIG_H) $(SYSTEM_H) \
+ coretypes.h $(GTM_H) $(RTL_BASE_H) errors.h $(READ_MD_H) gensupport.h \
+ $(HASH_TABLE_H) target-insns.def
build/gengenrtl.o : gengenrtl.c $(BCONFIG_H) $(SYSTEM_H) rtl.def
# The gengtype generator program is special: Two versions are built.
@@ -2562,7 +2566,7 @@ build/genmatch.o : genmatch.c $(BCONFIG_
# All these programs use the RTL reader ($(BUILD_RTL)).
genprogrtl = attr attr-common attrtab automata codes conditions config emit \
- extract flags opinit output peep preds recog mddump
+ extract flags mddump opinit output peep preds recog target-def
$(genprogrtl:%=build/gen%$(build_exeext)): $(BUILD_RTL)
# All these programs use the MD reader ($(BUILD_MD)).
Index: gcc/target-insns.def
===================================================================
--- /dev/null 2015-06-22 13:54:06.971314091 +0100
+++ gcc/target-insns.def 2015-06-22 14:04:26.690971419 +0100
@@ -0,0 +1,34 @@
+/* Target instruction definitions.
+ Copyright (C) 2015 Free Software Foundation, Inc.
+
+ This program is free software; you can redistribute it and/or modify it
+ under the terms of the GNU General Public License as published by the
+ Free Software Foundation; either version 3, or (at your option) any
+ later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License
+ along with this program; see the file COPYING3. If not see
+ <http://www.gnu.org/licenses/>. */
+
+/* This file has one entry for each public pattern name that the target
+ can provide. It is only used if no distinction between operand modes
+ is necessary. If separate patterns are needed for different modes
+ (so as to distinguish addition of QImode values from addition of
+ HImode values, for example) then an optab should be used instead.
+
+ Each entry has the form:
+
+ DEF_TARGET_INSN (name, prototype)
+
+ where NAME is the name of the pattern and PROTOTYPE is its C prototype.
+ The prototype should use parameter names of the form "x0", "x1", etc.
+ Patterns that take no operands should have a prototype "(void)".
+
+ Instructions should be documented in md.texi rather than here. */
+DEF_TARGET_INSN (return, (void))
+DEF_TARGET_INSN (simple_return, (void))
Index: gcc/gentarget-def.c
===================================================================
--- /dev/null 2015-06-22 13:54:06.971314091 +0100
+++ gcc/gentarget-def.c 2015-06-22 14:04:26.689971484 +0100
@@ -0,0 +1,265 @@
+/* Generate insn-target-def.h, an automatically-generated part of targetm.
+ Copyright (C) 1987-2015 Free Software Foundation, Inc.
+
+This file is part of GCC.
+
+GCC is free software; you can redistribute it and/or modify it under
+the terms of the GNU General Public License as published by the Free
+Software Foundation; either version 3, or (at your option) any later
+version.
+
+GCC is distributed in the hope that it will be useful, but WITHOUT ANY
+WARRANTY; without even the implied warranty of MERCHANTABILITY or
+FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+for more details.
+
+You should have received a copy of the GNU General Public License
+along with GCC; see the file COPYING3. If not see
+<http://www.gnu.org/licenses/>. */
+
+#include "bconfig.h"
+#include "system.h"
+#include "coretypes.h"
+#include "tm.h"
+#include "rtl.h"
+#include "errors.h"
+#include "read-md.h"
+#include "gensupport.h"
+#include "hash-table.h"
+
+/* This class hashes define_insns and define_expands by name. */
+struct insn_hasher : nofree_ptr_hash <rtx_def>
+{
+ typedef rtx value_type;
+ typedef const char *compare_type;
+
+ static inline hashval_t hash (rtx);
+ static inline bool equal (rtx, const char *);
+};
+
+hashval_t
+insn_hasher::hash (rtx x)
+{
+ return htab_hash_string (XSTR (x, 0));
+}
+
+bool
+insn_hasher::equal (rtx x, const char *y)
+{
+ return strcmp (XSTR (x, 0), y) == 0;
+}
+
+/* All define_insns and define_expands, hashed by name. */
+static hash_table <insn_hasher> *insns;
+
+/* Records the prototype suffix X for each invalid_X stub that has been
+ generated. */
+static hash_table <nofree_string_hash> *stubs;
+
+/* Records which C conditions have been wrapped in functions, as a mapping
+ from the C condition to the function name. */
+static hash_map <nofree_string_hash, const char *> *have_funcs;
+
+/* Output hook definitions for pattern NAME, which has target-insns.def
+ prototype PROTOTYPE. */
+
+static void
+def_target_insn (const char *name, const char *prototype)
+{
+ /* Get an upper-case form of NAME. */
+ unsigned int i;
+ char *upper_name = XALLOCAVEC (char, strlen (name) + 1);
+ for (i = 0; name[i]; ++i)
+ upper_name[i] = TOUPPER (name[i]);
+ upper_name[i] = 0;
+
+ /* Check that the prototype is valid and concatenate the types
+ together to get a suffix. */
+ char *suffix = XALLOCAVEC (char, strlen (prototype) + 1);
+ i = 0;
+ unsigned int opno = 0;
+ for (const char *p = prototype; *p; ++p)
+ if (*p == 'x' && ISDIGIT (p[1]))
+ {
+ /* This should be a parameter name of the form "x<OPNO>".
+ That doesn't contribute to the suffix, so skip ahead and
+ process the following character. */
+ char *endptr;
+ if (strtol (p + 1, &endptr, 10) != opno
+ || (*endptr != ',' && *endptr != ')'))
+ {
+ error ("invalid prototype for '%s'", name);
+ exit (FATAL_EXIT_CODE);
+ }
+ opno += 1;
+ p = endptr;
+ if (*p == ',')
+ suffix[i++] = '_';
+ }
+ else if (*p == ')' || *p == ',')
+ {
+ /* We found the end of a parameter without finding a
+ parameter name. */
+ if (strcmp (prototype, "(void)") != 0)
+ {
+ error ("argument %d of '%s' did not have the expected name",
+ opno, name);
+ exit (FATAL_EXIT_CODE);
+ }
+ }
+ else if (*p != '(' && !ISSPACE (*p))
+ suffix[i++] = *p;
+ suffix[i] = 0;
+
+ /* See whether we have an implementation of this pattern. */
+ hashval_t hash = htab_hash_string (name);
+ int truth = 0;
+ const char *have_name = name;
+ if (rtx insn = insns->find_with_hash (name, hash))
+ {
+ const char *test = XSTR (insn, 2);
+ truth = maybe_eval_c_test (test);
+ gcc_assert (truth != 0);
+ if (truth < 0)
+ {
+ /* Try to reuse an existing function that performs the same test. */
+ bool existed;
+ const char *&entry = have_funcs->get_or_insert (test, &existed);
+ if (!existed)
+ {
+ entry = name;
+ printf ("\nstatic bool\n");
+ printf ("target_have_%s (void)\n", name);
+ printf ("{\n");
+ printf (" return ");
+ print_c_condition (test);
+ printf (";\n");
+ printf ("}\n");
+ }
+ have_name = entry;
+ }
+ printf ("\nstatic rtx_insn *\n");
+ printf ("target_gen_%s %s\n", name, prototype);
+ printf ("{\n");
+ if (truth < 0)
+ printf (" gcc_checking_assert (targetm.have_%s ());\n", name);
+ printf (" return insnify (gen_%s (", name);
+ for (i = 0; i < opno; ++i)
+ printf ("%sx%d", i == 0 ? "" : ", ", i);
+ printf ("));\n");
+ printf ("}\n");
+ }
+ else
+ {
+ const char **slot = stubs->find_slot (suffix, INSERT);
+ if (!*slot)
+ {
+ *slot = xstrdup (suffix);
+ printf ("\nstatic rtx_insn *\n");
+ printf ("invalid_%s ", suffix);
+ const char *p = prototype;
+ while (*p)
+ {
+ if (p[0] == 'x' && ISDIGIT (p[1]))
+ {
+ char *endptr;
+ strtol (p + 1, &endptr, 10);
+ p = endptr;
+ }
+ else
+ fputc (*p++, stdout);
+ }
+ printf ("\n{\n");
+ printf (" gcc_unreachable ();\n");
+ printf ("}\n");
+ }
+ }
+ printf ("\n#undef TARGET_HAVE_%s\n", upper_name);
+ printf ("#define TARGET_HAVE_%s ", upper_name);
+ if (truth == 0)
+ printf ("hook_bool_void_false\n");
+ else if (truth == 1)
+ printf ("hook_bool_void_true\n");
+ else
+ printf ("target_have_%s\n", have_name);
+
+ printf ("#undef TARGET_GEN_%s\n", upper_name);
+ printf ("#define TARGET_GEN_%s ", upper_name);
+ if (truth == 0)
+ printf ("invalid_%s\n", suffix);
+ else
+ printf ("target_gen_%s\n", name);
+}
+
+int
+main (int argc, char **argv)
+{
+ int insn_code_number = 0;
+
+ progname = "gentarget-def";
+
+ if (!init_rtx_reader_args (argc, argv))
+ return (FATAL_EXIT_CODE);
+
+ insns = new hash_table <insn_hasher> (31);
+ stubs = new hash_table <nofree_string_hash> (31);
+ have_funcs = new hash_map <nofree_string_hash, const char *>;
+
+ while (1)
+ {
+ int line_no;
+ rtx desc = read_md_rtx (&line_no, &insn_code_number);
+ if (desc == NULL)
+ break;
+ if (GET_CODE (desc) == DEFINE_INSN || GET_CODE (desc) == DEFINE_EXPAND)
+ {
+ const char *name = XSTR (desc, 0);
+ if (name[0] != 0 && name[0] != '*')
+ {
+ hashval_t hash = htab_hash_string (name);
+ rtx *slot = insns->find_slot_with_hash (name, hash, INSERT);
+ if (*slot)
+ {
+ message_with_line (line_no, "duplicate definition of '%s'",
+ name);
+ have_error = 1;
+ }
+ else
+ *slot = desc;
+ }
+ }
+ }
+
+ printf ("/* Generated automatically by the program `gentarget-def'. */\n");
+ printf ("#ifndef GCC_INSN_TARGET_DEF_H\n");
+ printf ("#define GCC_INSN_TARGET_DEF_H\n");
+
+ /* Output a routine to convert an rtx to an rtx_insn sequence.
+ ??? At some point the gen_* functions themselves should return
+ rtx_insns. */
+ printf ("\nstatic inline rtx_insn *\n");
+ printf ("insnify (rtx x)\n");
+ printf ("{\n");
+ printf (" if (!x)\n");
+ printf (" return NULL;\n");
+ printf (" if (rtx_insn *insn = dyn_cast <rtx_insn *> (x))\n");
+ printf (" return insn;\n");
+ printf (" start_sequence ();\n");
+ printf (" emit (x);\n");
+ printf (" rtx_insn *res = get_insns ();\n");
+ printf (" end_sequence ();\n");
+ printf (" return res;\n");
+ printf ("}\n");
+
+#define DEF_TARGET_INSN(INSN, ARGS) \
+ def_target_insn (#INSN, #ARGS);
+#include "target-insns.def"
+#undef DEF_TARGET_INSN
+
+ printf ("\n#endif /* GCC_INSN_TARGET_DEF_H */\n");
+
+ if (have_error || ferror (stdout) || fflush (stdout) || fclose (stdout))
+ return FATAL_EXIT_CODE;
+
+ return SUCCESS_EXIT_CODE;
+}
Index: gcc/target.def
===================================================================
--- gcc/target.def 2015-06-22 14:03:10.985503368 +0100
+++ gcc/target.def 2015-06-22 14:04:26.690971419 +0100
@@ -5864,6 +5864,19 @@ DEFHOOK
HOOK_VECTOR_END (mode_switching)
+#undef HOOK_PREFIX
+#define HOOK_PREFIX "TARGET_"
+
+#define DEF_TARGET_INSN(NAME, PROTO) \
+ DEFHOOK_UNDOC (have_##NAME, "", bool, (void), false)
+#include "target-insns.def"
+#undef DEF_TARGET_INSN
+
+#define DEF_TARGET_INSN(NAME, PROTO) \
+ DEFHOOK_UNDOC (gen_##NAME, "", rtx_insn *, PROTO, NULL)
+#include "target-insns.def"
+#undef DEF_TARGET_INSN
+
/* Close the 'struct gcc_target' definition. */
HOOK_VECTOR_END (C90_EMPTY_HACK)
Index: gcc/defaults.h
===================================================================
--- gcc/defaults.h 2015-06-22 14:03:10.985503368 +0100
+++ gcc/defaults.h 2015-06-22 14:04:26.691971355 +0100
@@ -1426,26 +1426,6 @@ #define STACK_CHECK_MAX_VAR_SIZE (STACK_
#define TARGET_VTABLE_USES_DESCRIPTORS 0
#endif
-#ifndef HAVE_simple_return
-#define HAVE_simple_return 0
-static inline rtx
-gen_simple_return ()
-{
- gcc_unreachable ();
- return NULL;
-}
-#endif
-
-#ifndef HAVE_return
-#define HAVE_return 0
-static inline rtx
-gen_return ()
-{
- gcc_unreachable ();
- return NULL;
-}
-#endif
-
#ifndef HAVE_epilogue
#define HAVE_epilogue 0
static inline rtx
Index: gcc/target-def.h
===================================================================
--- gcc/target-def.h 2015-06-22 14:03:10.985503368 +0100
+++ gcc/target-def.h 2015-06-22 14:04:26.689971484 +0100
@@ -107,3 +107,4 @@ #define TARGET_FUNCTION_INCOMING_ARG TAR
#include "hooks.h"
#include "targhooks.h"
+#include "insn-target-def.h"
Index: gcc/cfgrtl.c
===================================================================
--- gcc/cfgrtl.c 2015-06-22 14:02:23.724223518 +0100
+++ gcc/cfgrtl.c 2015-06-22 14:04:26.693971225 +0100
@@ -1695,19 +1695,12 @@ force_nonfallthru_and_redirect (edge e,
if (target == EXIT_BLOCK_PTR_FOR_FN (cfun))
{
if (jump_label == ret_rtx)
- {
- if (!HAVE_return)
- gcc_unreachable ();
-
- emit_jump_insn_after_setloc (gen_return (), BB_END (jump_block), loc);
- }
+ emit_jump_insn_after_setloc (targetm.gen_return (),
+ BB_END (jump_block), loc);
else
{
gcc_assert (jump_label == simple_return_rtx);
- if (!HAVE_simple_return)
- gcc_unreachable ();
-
- emit_jump_insn_after_setloc (gen_simple_return (),
+ emit_jump_insn_after_setloc (targetm.gen_simple_return (),
BB_END (jump_block), loc);
}
set_return_jump_label (BB_END (jump_block));
Index: gcc/function.c
===================================================================
--- gcc/function.c 2015-06-22 14:04:03.369476746 +0100
+++ gcc/function.c 2015-06-22 14:04:26.692971290 +0100
@@ -5657,13 +5657,12 @@ emit_use_return_register_into_block (bas
/* Create a return pattern, either simple_return or return, depending on
simple_p. */
-static rtx
+static rtx_insn *
gen_return_pattern (bool simple_p)
{
- if (!HAVE_simple_return)
- gcc_assert (!simple_p);
-
- return simple_p ? gen_simple_return () : gen_return ();
+ return (simple_p
+ ? targetm.gen_simple_return ()
+ : targetm.gen_return ());
}
/* Insert an appropriate return pattern at the end of block BB. This
@@ -5769,7 +5768,7 @@ convert_jumps_to_returns (basic_block la
dest = ret_rtx;
if (!redirect_jump (as_a <rtx_jump_insn *> (jump), dest, 0))
{
- if (HAVE_simple_return && simple_p)
+ if (targetm.have_simple_return () && simple_p)
{
if (dump_file)
fprintf (dump_file,
@@ -5790,7 +5789,7 @@ convert_jumps_to_returns (basic_block la
}
else
{
- if (HAVE_simple_return && simple_p)
+ if (targetm.have_simple_return () && simple_p)
{
if (dump_file)
fprintf (dump_file,
@@ -5981,12 +5980,12 @@ thread_prologue_and_epilogue_insns (void
exit_fallthru_edge = find_fallthru_edge (EXIT_BLOCK_PTR_FOR_FN (cfun)->preds);
- if (HAVE_simple_return && entry_edge != orig_entry_edge)
+ if (targetm.have_simple_return () && entry_edge != orig_entry_edge)
exit_fallthru_edge
= get_unconverted_simple_return (exit_fallthru_edge, bb_flags,
&unconverted_simple_returns,
&returnjump);
- if (HAVE_return)
+ if (targetm.have_return ())
{
if (exit_fallthru_edge == NULL)
goto epilogue_done;
@@ -6007,7 +6006,8 @@ thread_prologue_and_epilogue_insns (void
/* Emitting the return may add a basic block.
Fix bb_flags for the added block. */
- if (HAVE_simple_return && last_bb != exit_fallthru_edge->src)
+ if (targetm.have_simple_return ()
+ && last_bb != exit_fallthru_edge->src)
bitmap_set_bit (&bb_flags, last_bb->index);
goto epilogue_done;
@@ -6123,7 +6123,7 @@ thread_prologue_and_epilogue_insns (void
}
}
- if (HAVE_simple_return)
+ if (targetm.have_simple_return ())
convert_to_simple_return (entry_edge, orig_entry_edge, bb_flags,
returnjump, unconverted_simple_returns);
@@ -6139,8 +6139,9 @@ thread_prologue_and_epilogue_insns (void
if (!CALL_P (insn)
|| ! SIBLING_CALL_P (insn)
- || (HAVE_simple_return && (entry_edge != orig_entry_edge
- && !bitmap_bit_p (&bb_flags, bb->index))))
+ || (targetm.have_simple_return ()
+ && entry_edge != orig_entry_edge
+ && !bitmap_bit_p (&bb_flags, bb->index)))
{
ei_next (&ei);
continue;
Index: gcc/reorg.c
===================================================================
--- gcc/reorg.c 2015-06-22 14:03:10.985503368 +0100
+++ gcc/reorg.c 2015-06-22 14:04:26.692971290 +0100
@@ -473,7 +473,7 @@ find_end_label (rtx kind)
}
else
{
- if (HAVE_epilogue && ! HAVE_return)
+ if (HAVE_epilogue && ! targetm.have_return ())
/* The RETURN insn has its delay slot filled so we cannot
emit the label just before it. Since we already have
an epilogue and cannot emit a new RETURN, we cannot
@@ -483,10 +483,10 @@ find_end_label (rtx kind)
/* Otherwise, make a new label and emit a RETURN and BARRIER,
if needed. */
emit_label (label);
- if (HAVE_return)
+ if (targetm.have_return ())
{
/* The return we make may have delay slots too. */
- rtx pat = gen_return ();
+ rtx_insn *pat = targetm.gen_return ();
rtx_insn *insn = emit_jump_insn (pat);
set_return_jump_label (insn);
emit_barrier ();
@@ -3815,8 +3815,9 @@ dbr_schedule (rtx_insn *first)
delete_related_insns (function_simple_return_label);
need_return_insns = false;
- need_return_insns |= HAVE_return && function_return_label != 0;
- need_return_insns |= HAVE_simple_return && function_simple_return_label != 0;
+ need_return_insns |= targetm.have_return () && function_return_label != 0;
+ need_return_insns |= (targetm.have_simple_return ()
+ && function_simple_return_label != 0);
if (need_return_insns)
make_return_insns (first);
Index: gcc/shrink-wrap.h
===================================================================
--- gcc/shrink-wrap.h 2015-06-22 14:03:10.985503368 +0100
+++ gcc/shrink-wrap.h 2015-06-22 14:04:26.692971290 +0100
@@ -36,7 +36,8 @@ extern void convert_to_simple_return (ed
bitmap_head bb_flags,
rtx_insn *returnjump,
vec<edge> unconverted_simple_returns);
-#define SHRINK_WRAPPING_ENABLED (flag_shrink_wrap && HAVE_simple_return)
+#define SHRINK_WRAPPING_ENABLED \
+ (flag_shrink_wrap && targetm.have_simple_return ())
#endif /* GCC_SHRINK_WRAP_H */
Index: gcc/shrink-wrap.c
===================================================================
--- gcc/shrink-wrap.c 2015-06-22 14:03:10.985503368 +0100
+++ gcc/shrink-wrap.c 2015-06-22 14:04:26.692971290 +0100
@@ -541,7 +541,7 @@ try_shrink_wrapping (edge *entry_edge, e
break;
}
- if (flag_shrink_wrap && HAVE_simple_return
+ if (SHRINK_WRAPPING_ENABLED
&& (targetm.profile_before_prologue () || !crtl->profile)
&& nonempty_prologue && !crtl->calls_eh_return)
{
@@ -1004,8 +1004,8 @@ convert_to_simple_return (edge entry_edg
bb = create_basic_block (NULL, NULL, exit_pred);
BB_COPY_PARTITION (bb, e->src);
- rtx_jump_insn *start = emit_jump_insn_after (gen_simple_return (),
- BB_END (bb));
+ rtx_insn *ret = targetm.gen_simple_return ();
+ rtx_jump_insn *start = emit_jump_insn_after (ret, BB_END (bb));
JUMP_LABEL (start) = simple_return_rtx;
emit_barrier_after (start);
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-23 18:42 Add .def file for public target instructions Richard Sandiford
@ 2015-06-24 6:36 ` Jeff Law
2015-06-25 20:11 ` H.J. Lu
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Jeff Law @ 2015-06-24 6:36 UTC (permalink / raw)
To: gcc-patches, Mikhail Maltsev, rdsandiford
On 06/23/2015 12:41 PM, Richard Sandiford wrote:
> [A fair bit later than promised, sorry...]
>
> Mikhail posted a patch to make genflags generate the default HAVE_foo
> and gen_foo definitions that have recently been added to defaults.h:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>
> I agree it'd be a good idea to generate this kind of thing automatically,
> but I think we should take the opportunity to move the interface to the
> target structure. I.e.:
>
> HAVE_foo -> targetm.have_foo ()
> gen_foo -> targetm.gen_foo ()
>
> This should move us closer to the pipedream goal of supporting multiple
> targets at once. It should also mean that only the target code depends
> on insn-flags.h.
>
> The patch just moves return and simple_return as an example. I have more
> locally (in order to test other code paths), but they're just an obvious
> extension of this one.
>
> The patch relies on the hashing changes in:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>
> and on this trivial patch:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>
> It seems a bit heavyweight when you just look at these two instructions,
> but I think it'll be a saving in the end.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
> via config-list.mk. OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> * Makefile.in (TARGET_DEF): Add target-insns.def.
> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
> (build/gentarget-def.o): New rule.
> (genprogrtl): Add target-def.
> * target-insns.def, gentarget-def.c: New files.
> * target.def: Add targetm.have_* and targetm.gen_* hooks,
> based on the contents of target-insns.def.
> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
> (HAVE_return, gen_return): Delete.
> * target-def.h: Include insn-target-def.h.
> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
> instead of direct calls. Rely on them to do the appropriate assertions.
> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
> (convert_jumps_to_returns): Use targetm interface instead of
> direct calls.
> (thread_prologue_and_epilogue_insns): Likewise.
> * reorg.c (find_end_label, dbr_schedule): Likewise.
> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
> * shrink-wrap.c (convert_to_simple_return): Likewise.
> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
As you're the gen* maintainer, I'm going to assume you got those bits
right and if there's a problem you'll address them :-)
This is fine once the prereqs are installed. Follow-ups to move more
insns to this style are pre-approved assuming their of a similar
mechanical nature.
jeff
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-23 18:42 Add .def file for public target instructions Richard Sandiford
2015-06-24 6:36 ` Jeff Law
@ 2015-06-25 20:11 ` H.J. Lu
2015-06-25 23:00 ` H.J. Lu
2015-06-26 7:42 ` Markus Trippelsdorf
2015-07-01 9:39 ` Trevor Saunders
3 siblings, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2015-06-25 20:11 UTC (permalink / raw)
To: GCC Patches, Mikhail Maltsev, Richard Sandiford
On Tue, Jun 23, 2015 at 11:41 AM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> [A fair bit later than promised, sorry...]
>
> Mikhail posted a patch to make genflags generate the default HAVE_foo
> and gen_foo definitions that have recently been added to defaults.h:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>
> I agree it'd be a good idea to generate this kind of thing automatically,
> but I think we should take the opportunity to move the interface to the
> target structure. I.e.:
>
> HAVE_foo -> targetm.have_foo ()
> gen_foo -> targetm.gen_foo ()
>
> This should move us closer to the pipedream goal of supporting multiple
> targets at once. It should also mean that only the target code depends
> on insn-flags.h.
>
> The patch just moves return and simple_return as an example. I have more
> locally (in order to test other code paths), but they're just an obvious
> extension of this one.
>
> The patch relies on the hashing changes in:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>
> and on this trivial patch:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>
> It seems a bit heavyweight when you just look at these two instructions,
> but I think it'll be a saving in the end.
>
> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
> via config-list.mk. OK to install?
>
> Thanks,
> Richard
>
>
> gcc/
> * Makefile.in (TARGET_DEF): Add target-insns.def.
> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
> (build/gentarget-def.o): New rule.
> (genprogrtl): Add target-def.
> * target-insns.def, gentarget-def.c: New files.
> * target.def: Add targetm.have_* and targetm.gen_* hooks,
> based on the contents of target-insns.def.
> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
> (HAVE_return, gen_return): Delete.
> * target-def.h: Include insn-target-def.h.
> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
> instead of direct calls. Rely on them to do the appropriate assertions.
> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
> (convert_jumps_to_returns): Use targetm interface instead of
> direct calls.
> (thread_prologue_and_epilogue_insns): Likewise.
> * reorg.c (find_end_label, dbr_schedule): Likewise.
> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
> * shrink-wrap.c (convert_to_simple_return): Likewise.
> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>
This breaks bootstrap on Linux/ia32:
https://gcc.gnu.org/ml/gcc-regression/2015-06/msg00649.html
../../src-trunk/gcc/gentarget-def.c: In function âvoid
def_target_insn(const char*, const char*)â:
../../src-trunk/gcc/gentarget-def.c:88:34: error: comparison between
signed and unsigned integer expressions [-Werror=sign-compare]
if (strtol (p + 1, &endptr, 10) != opno
--
H.J.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-25 20:11 ` H.J. Lu
@ 2015-06-25 23:00 ` H.J. Lu
2015-06-25 23:37 ` Andrew Pinski
0 siblings, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2015-06-25 23:00 UTC (permalink / raw)
To: GCC Patches, Mikhail Maltsev, Richard Sandiford
On Thu, Jun 25, 2015 at 1:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Tue, Jun 23, 2015 at 11:41 AM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> [A fair bit later than promised, sorry...]
>>
>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>> and gen_foo definitions that have recently been added to defaults.h:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>
>> I agree it'd be a good idea to generate this kind of thing automatically,
>> but I think we should take the opportunity to move the interface to the
>> target structure. I.e.:
>>
>> HAVE_foo -> targetm.have_foo ()
>> gen_foo -> targetm.gen_foo ()
>>
>> This should move us closer to the pipedream goal of supporting multiple
>> targets at once. It should also mean that only the target code depends
>> on insn-flags.h.
>>
>> The patch just moves return and simple_return as an example. I have more
>> locally (in order to test other code paths), but they're just an obvious
>> extension of this one.
>>
>> The patch relies on the hashing changes in:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>>
>> and on this trivial patch:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>>
>> It seems a bit heavyweight when you just look at these two instructions,
>> but I think it'll be a saving in the end.
>>
>> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
>> via config-list.mk. OK to install?
>>
>> Thanks,
>> Richard
>>
>>
>> gcc/
>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>> (build/gentarget-def.o): New rule.
>> (genprogrtl): Add target-def.
>> * target-insns.def, gentarget-def.c: New files.
>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>> based on the contents of target-insns.def.
>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>> (HAVE_return, gen_return): Delete.
>> * target-def.h: Include insn-target-def.h.
>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>> instead of direct calls. Rely on them to do the appropriate assertions.
>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>> (convert_jumps_to_returns): Use targetm interface instead of
>> direct calls.
>> (thread_prologue_and_epilogue_insns): Likewise.
>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>>
>
> This breaks bootstrap on Linux/ia32:
>
> https://gcc.gnu.org/ml/gcc-regression/2015-06/msg00649.html
>
> ../../src-trunk/gcc/gentarget-def.c: In function âvoid
> def_target_insn(const char*, const char*)â:
> ../../src-trunk/gcc/gentarget-def.c:88:34: error: comparison between
> signed and unsigned integer expressions [-Werror=sign-compare]
> if (strtol (p + 1, &endptr, 10) != opno
>
There are
unsigned int opno = 0;
for (const char *p = prototype; *p; ++p)
if (*p == 'x' && ISDIGIT (p[1]))
{
/* This should be a parameter name of the form "x<OPNO>".
That doesn't contribute to the suffix, so skip ahead and
process the following character. */
char *endptr;
if (strtol (p + 1, &endptr, 10) != opno
|| (*endptr != ',' && *endptr != ')'))
strtol returns long int. Somehow, there is no warning on x86-64.
--
H.J.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-25 23:00 ` H.J. Lu
@ 2015-06-25 23:37 ` Andrew Pinski
2015-06-25 23:55 ` H.J. Lu
0 siblings, 1 reply; 15+ messages in thread
From: Andrew Pinski @ 2015-06-25 23:37 UTC (permalink / raw)
To: H.J. Lu; +Cc: GCC Patches, Mikhail Maltsev, Richard Sandiford
On Thu, Jun 25, 2015 at 3:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jun 25, 2015 at 1:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Tue, Jun 23, 2015 at 11:41 AM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> [A fair bit later than promised, sorry...]
>>>
>>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>>> and gen_foo definitions that have recently been added to defaults.h:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>>
>>> I agree it'd be a good idea to generate this kind of thing automatically,
>>> but I think we should take the opportunity to move the interface to the
>>> target structure. I.e.:
>>>
>>> HAVE_foo -> targetm.have_foo ()
>>> gen_foo -> targetm.gen_foo ()
>>>
>>> This should move us closer to the pipedream goal of supporting multiple
>>> targets at once. It should also mean that only the target code depends
>>> on insn-flags.h.
>>>
>>> The patch just moves return and simple_return as an example. I have more
>>> locally (in order to test other code paths), but they're just an obvious
>>> extension of this one.
>>>
>>> The patch relies on the hashing changes in:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>>>
>>> and on this trivial patch:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>>>
>>> It seems a bit heavyweight when you just look at these two instructions,
>>> but I think it'll be a saving in the end.
>>>
>>> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
>>> via config-list.mk. OK to install?
>>>
>>> Thanks,
>>> Richard
>>>
>>>
>>> gcc/
>>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>>> (build/gentarget-def.o): New rule.
>>> (genprogrtl): Add target-def.
>>> * target-insns.def, gentarget-def.c: New files.
>>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>>> based on the contents of target-insns.def.
>>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>>> (HAVE_return, gen_return): Delete.
>>> * target-def.h: Include insn-target-def.h.
>>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>>> instead of direct calls. Rely on them to do the appropriate assertions.
>>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>>> (convert_jumps_to_returns): Use targetm interface instead of
>>> direct calls.
>>> (thread_prologue_and_epilogue_insns): Likewise.
>>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>>>
>>
>> This breaks bootstrap on Linux/ia32:
>>
>> https://gcc.gnu.org/ml/gcc-regression/2015-06/msg00649.html
>>
>> ../../src-trunk/gcc/gentarget-def.c: In function âvoid
>> def_target_insn(const char*, const char*)â:
>> ../../src-trunk/gcc/gentarget-def.c:88:34: error: comparison between
>> signed and unsigned integer expressions [-Werror=sign-compare]
>> if (strtol (p + 1, &endptr, 10) != opno
>>
>
> There are
>
> unsigned int opno = 0;
> for (const char *p = prototype; *p; ++p)
> if (*p == 'x' && ISDIGIT (p[1]))
> {
> /* This should be a parameter name of the form "x<OPNO>".
> That doesn't contribute to the suffix, so skip ahead and
> process the following character. */
> char *endptr;
> if (strtol (p + 1, &endptr, 10) != opno
> || (*endptr != ',' && *endptr != ')'))
>
> strtol returns long int. Somehow, there is no warning on x86-64.
Because on x86_64 (and all LP64 targets), the comparison gets promoted
to long (which is 64bit) so the conversion from unsigned int to long
does not lose precision.
Thanks,
Andrew
>
> --
> H.J.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-25 23:37 ` Andrew Pinski
@ 2015-06-25 23:55 ` H.J. Lu
2015-06-26 6:33 ` H.J. Lu
0 siblings, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2015-06-25 23:55 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches, Mikhail Maltsev, Richard Sandiford
On Thu, Jun 25, 2015 at 4:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
> On Thu, Jun 25, 2015 at 3:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Jun 25, 2015 at 1:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Tue, Jun 23, 2015 at 11:41 AM, Richard Sandiford
>>> <rdsandiford@googlemail.com> wrote:
>>>> [A fair bit later than promised, sorry...]
>>>>
>>>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>>>> and gen_foo definitions that have recently been added to defaults.h:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>>>
>>>> I agree it'd be a good idea to generate this kind of thing automatically,
>>>> but I think we should take the opportunity to move the interface to the
>>>> target structure. I.e.:
>>>>
>>>> HAVE_foo -> targetm.have_foo ()
>>>> gen_foo -> targetm.gen_foo ()
>>>>
>>>> This should move us closer to the pipedream goal of supporting multiple
>>>> targets at once. It should also mean that only the target code depends
>>>> on insn-flags.h.
>>>>
>>>> The patch just moves return and simple_return as an example. I have more
>>>> locally (in order to test other code paths), but they're just an obvious
>>>> extension of this one.
>>>>
>>>> The patch relies on the hashing changes in:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>>>>
>>>> and on this trivial patch:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>>>>
>>>> It seems a bit heavyweight when you just look at these two instructions,
>>>> but I think it'll be a saving in the end.
>>>>
>>>> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
>>>> via config-list.mk. OK to install?
>>>>
>>>> Thanks,
>>>> Richard
>>>>
>>>>
>>>> gcc/
>>>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>>>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>>>> (build/gentarget-def.o): New rule.
>>>> (genprogrtl): Add target-def.
>>>> * target-insns.def, gentarget-def.c: New files.
>>>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>>>> based on the contents of target-insns.def.
>>>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>>>> (HAVE_return, gen_return): Delete.
>>>> * target-def.h: Include insn-target-def.h.
>>>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>>>> instead of direct calls. Rely on them to do the appropriate assertions.
>>>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>>>> (convert_jumps_to_returns): Use targetm interface instead of
>>>> direct calls.
>>>> (thread_prologue_and_epilogue_insns): Likewise.
>>>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>>>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>>>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>>>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>>>>
>>>
>>> This breaks bootstrap on Linux/ia32:
>>>
>>> https://gcc.gnu.org/ml/gcc-regression/2015-06/msg00649.html
>>>
>>> ../../src-trunk/gcc/gentarget-def.c: In function âvoid
>>> def_target_insn(const char*, const char*)â:
>>> ../../src-trunk/gcc/gentarget-def.c:88:34: error: comparison between
>>> signed and unsigned integer expressions [-Werror=sign-compare]
>>> if (strtol (p + 1, &endptr, 10) != opno
>>>
>>
>> There are
>>
>> unsigned int opno = 0;
>> for (const char *p = prototype; *p; ++p)
>> if (*p == 'x' && ISDIGIT (p[1]))
>> {
>> /* This should be a parameter name of the form "x<OPNO>".
>> That doesn't contribute to the suffix, so skip ahead and
>> process the following character. */
>> char *endptr;
>> if (strtol (p + 1, &endptr, 10) != opno
>> || (*endptr != ',' && *endptr != ')'))
>>
>> strtol returns long int. Somehow, there is no warning on x86-64.
>
> Because on x86_64 (and all LP64 targets), the comparison gets promoted
> to long (which is 64bit) so the conversion from unsigned int to long
> does not lose precision.
>
I am testing this.
--
H.J.
---
diff --git a/gcc/gentarget-def.c b/gcc/gentarget-def.c
index d4839e8..3ca9cfd 100644
--- a/gcc/gentarget-def.c
+++ b/gcc/gentarget-def.c
@@ -77,7 +77,7 @@ def_target_insn (const char *name, const char *prototype)
together to get a suffix. */
char *suffix = XALLOCAVEC (char, strlen (prototype) + 1);
i = 0;
- unsigned int opno = 0;
+ long opno = 0;
for (const char *p = prototype; *p; ++p)
if (*p == 'x' && ISDIGIT (p[1]))
{
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-25 23:55 ` H.J. Lu
@ 2015-06-26 6:33 ` H.J. Lu
2015-06-26 8:50 ` Richard Sandiford
0 siblings, 1 reply; 15+ messages in thread
From: H.J. Lu @ 2015-06-26 6:33 UTC (permalink / raw)
To: Andrew Pinski; +Cc: GCC Patches, Mikhail Maltsev, Richard Sandiford
On Thu, Jun 25, 2015 at 4:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jun 25, 2015 at 4:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>> On Thu, Jun 25, 2015 at 3:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>> On Thu, Jun 25, 2015 at 1:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Tue, Jun 23, 2015 at 11:41 AM, Richard Sandiford
>>>> <rdsandiford@googlemail.com> wrote:
>>>>> [A fair bit later than promised, sorry...]
>>>>>
>>>>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>>>>> and gen_foo definitions that have recently been added to defaults.h:
>>>>>
>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>>>>
>>>>> I agree it'd be a good idea to generate this kind of thing automatically,
>>>>> but I think we should take the opportunity to move the interface to the
>>>>> target structure. I.e.:
>>>>>
>>>>> HAVE_foo -> targetm.have_foo ()
>>>>> gen_foo -> targetm.gen_foo ()
>>>>>
>>>>> This should move us closer to the pipedream goal of supporting multiple
>>>>> targets at once. It should also mean that only the target code depends
>>>>> on insn-flags.h.
>>>>>
>>>>> The patch just moves return and simple_return as an example. I have more
>>>>> locally (in order to test other code paths), but they're just an obvious
>>>>> extension of this one.
>>>>>
>>>>> The patch relies on the hashing changes in:
>>>>>
>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>>>>>
>>>>> and on this trivial patch:
>>>>>
>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>>>>>
>>>>> It seems a bit heavyweight when you just look at these two instructions,
>>>>> but I think it'll be a saving in the end.
>>>>>
>>>>> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
>>>>> via config-list.mk. OK to install?
>>>>>
>>>>> Thanks,
>>>>> Richard
>>>>>
>>>>>
>>>>> gcc/
>>>>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>>>>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>>>>> (build/gentarget-def.o): New rule.
>>>>> (genprogrtl): Add target-def.
>>>>> * target-insns.def, gentarget-def.c: New files.
>>>>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>>>>> based on the contents of target-insns.def.
>>>>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>>>>> (HAVE_return, gen_return): Delete.
>>>>> * target-def.h: Include insn-target-def.h.
>>>>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>>>>> instead of direct calls. Rely on them to do the appropriate assertions.
>>>>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>>>>> (convert_jumps_to_returns): Use targetm interface instead of
>>>>> direct calls.
>>>>> (thread_prologue_and_epilogue_insns): Likewise.
>>>>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>>>>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>>>>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>>>>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>>>>>
>>>>
>>>> This breaks bootstrap on Linux/ia32:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-regression/2015-06/msg00649.html
>>>>
>>>> ../../src-trunk/gcc/gentarget-def.c: In function âvoid
>>>> def_target_insn(const char*, const char*)â:
>>>> ../../src-trunk/gcc/gentarget-def.c:88:34: error: comparison between
>>>> signed and unsigned integer expressions [-Werror=sign-compare]
>>>> if (strtol (p + 1, &endptr, 10) != opno
>>>>
>>>
>>> There are
>>>
>>> unsigned int opno = 0;
>>> for (const char *p = prototype; *p; ++p)
>>> if (*p == 'x' && ISDIGIT (p[1]))
>>> {
>>> /* This should be a parameter name of the form "x<OPNO>".
>>> That doesn't contribute to the suffix, so skip ahead and
>>> process the following character. */
>>> char *endptr;
>>> if (strtol (p + 1, &endptr, 10) != opno
>>> || (*endptr != ',' && *endptr != ')'))
>>>
>>> strtol returns long int. Somehow, there is no warning on x86-64.
>>
>> Because on x86_64 (and all LP64 targets), the comparison gets promoted
>> to long (which is 64bit) so the conversion from unsigned int to long
>> does not lose precision.
>>
>
> I am testing this.
>
>
> --
> H.J.
> ---
> diff --git a/gcc/gentarget-def.c b/gcc/gentarget-def.c
> index d4839e8..3ca9cfd 100644
> --- a/gcc/gentarget-def.c
> +++ b/gcc/gentarget-def.c
> @@ -77,7 +77,7 @@ def_target_insn (const char *name, const char *prototype)
> together to get a suffix. */
> char *suffix = XALLOCAVEC (char, strlen (prototype) + 1);
> i = 0;
> - unsigned int opno = 0;
> + long opno = 0;
> for (const char *p = prototype; *p; ++p)
> if (*p == 'x' && ISDIGIT (p[1]))
> {
It doesn't work. I checked in this patch instead.
--
H.J.
---
Index: ChangeLog
===================================================================
--- ChangeLog (revision 224992)
+++ ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2015-06-25 H.J. Lu <hongjiu.lu@intel.com>
+
+ * gentarget-def.c (def_target_insn): Cast return of strtol to
+ unsigned int.
+
2015-06-25 Andrew MacLeod <amacleod@redhat.com>
* gimple.h (gimple_call_set_fn): Move inline function.
Index: gentarget-def.c
===================================================================
--- gentarget-def.c (revision 224992)
+++ gentarget-def.c (working copy)
@@ -85,7 +85,7 @@ def_target_insn (const char *name, const
That doesn't contribute to the suffix, so skip ahead and
process the following character. */
char *endptr;
- if (strtol (p + 1, &endptr, 10) != opno
+ if ((unsigned int) strtol (p + 1, &endptr, 10) != opno
|| (*endptr != ',' && *endptr != ')'))
{
error ("invalid prototype for '%s'", name);
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-26 6:33 ` H.J. Lu
@ 2015-06-26 8:50 ` Richard Sandiford
0 siblings, 0 replies; 15+ messages in thread
From: Richard Sandiford @ 2015-06-26 8:50 UTC (permalink / raw)
To: H.J. Lu; +Cc: Andrew Pinski, GCC Patches, Mikhail Maltsev
"H.J. Lu" <hjl.tools@gmail.com> writes:
> On Thu, Jun 25, 2015 at 4:37 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Thu, Jun 25, 2015 at 4:00 PM, Andrew Pinski <pinskia@gmail.com> wrote:
>>> On Thu, Jun 25, 2015 at 3:57 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>> On Thu, Jun 25, 2015 at 1:09 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>>>> On Tue, Jun 23, 2015 at 11:41 AM, Richard Sandiford
>>>>> <rdsandiford@googlemail.com> wrote:
>>>>>> [A fair bit later than promised, sorry...]
>>>>>>
>>>>>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>>>>>> and gen_foo definitions that have recently been added to defaults.h:
>>>>>>
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>>>>>
>>>>>> I agree it'd be a good idea to generate this kind of thing automatically,
>>>>>> but I think we should take the opportunity to move the interface to the
>>>>>> target structure. I.e.:
>>>>>>
>>>>>> HAVE_foo -> targetm.have_foo ()
>>>>>> gen_foo -> targetm.gen_foo ()
>>>>>>
>>>>>> This should move us closer to the pipedream goal of supporting multiple
>>>>>> targets at once. It should also mean that only the target code depends
>>>>>> on insn-flags.h.
>>>>>>
>>>>>> The patch just moves return and simple_return as an example. I have more
>>>>>> locally (in order to test other code paths), but they're just an obvious
>>>>>> extension of this one.
>>>>>>
>>>>>> The patch relies on the hashing changes in:
>>>>>>
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01066.html
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01564.html
>>>>>>
>>>>>> and on this trivial patch:
>>>>>>
>>>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg01604.html
>>>>>>
>>>>>> It seems a bit heavyweight when you just look at these two instructions,
>>>>>> but I think it'll be a saving in the end.
>>>>>>
>>>>>> Bootstrapped & regression-tested on x86_64-linux-gnu. Also tested
>>>>>> via config-list.mk. OK to install?
>>>>>>
>>>>>> Thanks,
>>>>>> Richard
>>>>>>
>>>>>>
>>>>>> gcc/
>>>>>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>>>>>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>>>>>> (build/gentarget-def.o): New rule.
>>>>>> (genprogrtl): Add target-def.
>>>>>> * target-insns.def, gentarget-def.c: New files.
>>>>>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>>>>>> based on the contents of target-insns.def.
>>>>>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>>>>>> (HAVE_return, gen_return): Delete.
>>>>>> * target-def.h: Include insn-target-def.h.
>>>>>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>>>>>> instead of direct calls. Rely on them to do the appropriate assertions.
>>>>>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>>>>>> (convert_jumps_to_returns): Use targetm interface instead of
>>>>>> direct calls.
>>>>>> (thread_prologue_and_epilogue_insns): Likewise.
>>>>>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>>>>>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>>>>>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>>>>>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>>>>>>
>>>>>
>>>>> This breaks bootstrap on Linux/ia32:
>>>>>
>>>>> https://gcc.gnu.org/ml/gcc-regression/2015-06/msg00649.html
>>>>>
>>>>> ../../src-trunk/gcc/gentarget-def.c: In function âvoid
>>>>> def_target_insn(const char*, const char*)â:
>>>>> ../../src-trunk/gcc/gentarget-def.c:88:34: error: comparison between
>>>>> signed and unsigned integer expressions [-Werror=sign-compare]
>>>>> if (strtol (p + 1, &endptr, 10) != opno
>>>>>
>>>>
>>>> There are
>>>>
>>>> unsigned int opno = 0;
>>>> for (const char *p = prototype; *p; ++p)
>>>> if (*p == 'x' && ISDIGIT (p[1]))
>>>> {
>>>> /* This should be a parameter name of the form "x<OPNO>".
>>>> That doesn't contribute to the suffix, so skip ahead and
>>>> process the following character. */
>>>> char *endptr;
>>>> if (strtol (p + 1, &endptr, 10) != opno
>>>> || (*endptr != ',' && *endptr != ')'))
>>>>
>>>> strtol returns long int. Somehow, there is no warning on x86-64.
>>>
>>> Because on x86_64 (and all LP64 targets), the comparison gets promoted
>>> to long (which is 64bit) so the conversion from unsigned int to long
>>> does not lose precision.
>>>
>>
>> I am testing this.
>>
>>
>> --
>> H.J.
>> ---
>> diff --git a/gcc/gentarget-def.c b/gcc/gentarget-def.c
>> index d4839e8..3ca9cfd 100644
>> --- a/gcc/gentarget-def.c
>> +++ b/gcc/gentarget-def.c
>> @@ -77,7 +77,7 @@ def_target_insn (const char *name, const char *prototype)
>> together to get a suffix. */
>> char *suffix = XALLOCAVEC (char, strlen (prototype) + 1);
>> i = 0;
>> - unsigned int opno = 0;
>> + long opno = 0;
>> for (const char *p = prototype; *p; ++p)
>> if (*p == 'x' && ISDIGIT (p[1]))
>> {
>
> It doesn't work. I checked in this patch instead.
Thanks H.J., and sorry for the breakage.
Richard
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-23 18:42 Add .def file for public target instructions Richard Sandiford
2015-06-24 6:36 ` Jeff Law
2015-06-25 20:11 ` H.J. Lu
@ 2015-06-26 7:42 ` Markus Trippelsdorf
2015-06-26 8:45 ` Richard Sandiford
2015-07-01 9:39 ` Trevor Saunders
3 siblings, 1 reply; 15+ messages in thread
From: Markus Trippelsdorf @ 2015-06-26 7:42 UTC (permalink / raw)
To: gcc-patches, Mikhail Maltsev, rdsandiford
On 2015.06.23 at 19:41 +0100, Richard Sandiford wrote:
>
> gcc/
> * Makefile.in (TARGET_DEF): Add target-insns.def.
> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
> (build/gentarget-def.o): New rule.
> (genprogrtl): Add target-def.
> * target-insns.def, gentarget-def.c: New files.
> * target.def: Add targetm.have_* and targetm.gen_* hooks,
> based on the contents of target-insns.def.
> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
> (HAVE_return, gen_return): Delete.
> * target-def.h: Include insn-target-def.h.
> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
> instead of direct calls. Rely on them to do the appropriate assertions.
> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
> (convert_jumps_to_returns): Use targetm interface instead of
> direct calls.
> (thread_prologue_and_epilogue_insns): Likewise.
> * reorg.c (find_end_label, dbr_schedule): Likewise.
> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
> * shrink-wrap.c (convert_to_simple_return): Likewise.
> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
The patch breaks bootstrap on ppc64le. During libgcc configuration:
conftest.c: In function 'main':
conftest.c:16:1: internal compiler error: in as_a, at is-a.h:192
}
^
0x1010411b as_a<rtx_jump_insn*, rtx_insn>
../../gcc/gcc/is-a.h:192
0x1040ccd3 as_a<rtx_jump_insn*, rtx_insn>
../../gcc/gcc/emit-rtl.c:4750
0x1040ccd3 emit_jump_insn_after(rtx_def*, rtx_def*)
../../gcc/gcc/emit-rtl.c:4749
0x104c378f emit_return_into_block(bool, basic_block_def*)
../../gcc/gcc/function.c:5633
0x104c3ee7 emit_return_for_exit(edge_def*, bool)
../../gcc/gcc/function.c:5779
0x104c9ee3 thread_prologue_and_epilogue_insns()
../../gcc/gcc/function.c:5961
0x104ca39f rest_of_handle_thread_prologue_and_epilogue
../../gcc/gcc/function.c:6443
0x104ca39f execute
../../gcc/gcc/function.c:6481
Please submit a full bug report,
with preprocessed source if appropriate.
--
Markus
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-26 7:42 ` Markus Trippelsdorf
@ 2015-06-26 8:45 ` Richard Sandiford
2015-06-26 10:15 ` Richard Sandiford
0 siblings, 1 reply; 15+ messages in thread
From: Richard Sandiford @ 2015-06-26 8:45 UTC (permalink / raw)
To: Markus Trippelsdorf; +Cc: gcc-patches, Mikhail Maltsev
Markus Trippelsdorf <markus@trippelsdorf.de> writes:
> On 2015.06.23 at 19:41 +0100, Richard Sandiford wrote:
>>
>> gcc/
>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>> (build/gentarget-def.o): New rule.
>> (genprogrtl): Add target-def.
>> * target-insns.def, gentarget-def.c: New files.
>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>> based on the contents of target-insns.def.
>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>> (HAVE_return, gen_return): Delete.
>> * target-def.h: Include insn-target-def.h.
>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>> instead of direct calls. Rely on them to do the appropriate assertions.
>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>> (convert_jumps_to_returns): Use targetm interface instead of
>> direct calls.
>> (thread_prologue_and_epilogue_insns): Likewise.
>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>
> The patch breaks bootstrap on ppc64le. During libgcc configuration:
Yeah, sorry about that. The problem is due to confusion about whether
the generator should emit a barrier or not. After the final instruction
in a sequence, it should always be up to the caller to insert any
necessary barriers, just like it is for define_insns, and for define_expands
implemented in C++ with DONE.
I'm testing the following patch.
Thanks,
Richard
gcc/
* rtl.h (emit): Add an optional boolean parameter to control
whether barriers are emitted.
* emit-rtl.c (emit): Likewise.
* gensupport.c (get_emit_function): Return null rather than "emit".
* genemit.c (gen_emit_seq): Handle the null return value.
Don't emit barriers after the final instruction in the sequence.
* gentarget-def.c (main): Don't emit barriers after the instruction.
Index: gcc/rtl.h
===================================================================
--- gcc/rtl.h 2015-06-26 09:09:10.128602409 +0100
+++ gcc/rtl.h 2015-06-26 09:09:10.316600233 +0100
@@ -3494,7 +3494,7 @@ extern void add_insn (rtx_insn *);
extern void add_insn_before (rtx, rtx, basic_block);
extern void add_insn_after (rtx, rtx, basic_block);
extern void remove_insn (rtx);
-extern rtx_insn *emit (rtx);
+extern rtx_insn *emit (rtx, bool = true);
extern void emit_insn_at_entry (rtx);
extern rtx gen_lowpart_SUBREG (machine_mode, rtx);
extern rtx gen_const_mem (machine_mode, rtx);
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c 2015-06-26 09:09:10.124602455 +0100
+++ gcc/emit-rtl.c 2015-06-26 09:09:10.316600233 +0100
@@ -5303,11 +5303,14 @@ set_dst_reg_note (rtx insn, enum reg_not
return NULL_RTX;
}
\f
-/* Emit the rtl pattern X as an appropriate kind of insn.
+/* Emit the rtl pattern X as an appropriate kind of insn. Also emit a
+ following barrier if the instruction needs one and if ALLOW_BARRIER_P
+ is true.
+
If X is a label, it is simply added into the insn chain. */
rtx_insn *
-emit (rtx x)
+emit (rtx x, bool allow_barrier_p)
{
enum rtx_code code = classify_insn (x);
@@ -5320,7 +5323,8 @@ emit (rtx x)
case JUMP_INSN:
{
rtx_insn *insn = emit_jump_insn (x);
- if (any_uncondjump_p (insn) || GET_CODE (x) == RETURN)
+ if (allow_barrier_p
+ && (any_uncondjump_p (insn) || GET_CODE (x) == RETURN))
return emit_barrier ();
return insn;
}
Index: gcc/gensupport.c
===================================================================
--- gcc/gensupport.c 2015-06-26 09:09:10.124602455 +0100
+++ gcc/gensupport.c 2015-06-26 09:09:10.316600233 +0100
@@ -2983,7 +2983,9 @@ get_pattern_stats (struct pattern_stats
stats->max_scratch_opno)) + 1;
}
-/* Return the emit_* function that should be used for pattern X. */
+/* Return the emit_* function that should be used for pattern X, or NULL
+ if we can't pick a particular type at compile time and should instead
+ fall back to "emit". */
const char *
get_emit_function (rtx x)
@@ -3000,7 +3002,7 @@ get_emit_function (rtx x)
return "emit_jump_insn";
case UNKNOWN:
- return "emit";
+ return NULL;
default:
gcc_unreachable ();
Index: gcc/genemit.c
===================================================================
--- gcc/genemit.c 2015-06-26 09:09:10.124602455 +0100
+++ gcc/genemit.c 2015-06-26 09:09:10.316600233 +0100
@@ -277,12 +277,22 @@ gen_emit_seq (rtvec vec, char *used)
{
for (int i = 0, len = GET_NUM_ELEM (vec); i < len; ++i)
{
+ bool last_p = (i == len - 1);
rtx next = RTVEC_ELT (vec, i);
- printf (" %s (", get_emit_function (next));
- gen_exp (next, DEFINE_EXPAND, used);
- printf (");\n");
- if (needs_barrier_p (next))
- printf (" emit_barrier ();");
+ if (const char *name = get_emit_function (next))
+ {
+ printf (" %s (", name);
+ gen_exp (next, DEFINE_EXPAND, used);
+ printf (");\n");
+ if (!last_p && needs_barrier_p (next))
+ printf (" emit_barrier ();");
+ }
+ else
+ {
+ printf (" emit (");
+ gen_exp (next, DEFINE_EXPAND, used);
+ printf (", %s);\n", last_p ? "false" : "true");
+ }
}
}
\f
Index: gcc/gentarget-def.c
===================================================================
--- gcc/gentarget-def.c 2015-06-26 09:09:10.124602455 +0100
+++ gcc/gentarget-def.c 2015-06-26 09:09:10.316600233 +0100
@@ -245,7 +245,7 @@ main (int argc, char **argv)
printf (" if (rtx_insn *insn = dyn_cast <rtx_insn *> (x))\n");
printf (" return insn;\n");
printf (" start_sequence ();\n");
- printf (" emit (x);\n");
+ printf (" emit (x, false);\n");
printf (" rtx_insn *res = get_insns ();\n");
printf (" end_sequence ();\n");
printf (" return res;\n");
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-26 8:45 ` Richard Sandiford
@ 2015-06-26 10:15 ` Richard Sandiford
0 siblings, 0 replies; 15+ messages in thread
From: Richard Sandiford @ 2015-06-26 10:15 UTC (permalink / raw)
To: Markus Trippelsdorf; +Cc: gcc-patches, Mikhail Maltsev
Richard Sandiford <richard.sandiford@arm.com> writes:
> Markus Trippelsdorf <markus@trippelsdorf.de> writes:
>> On 2015.06.23 at 19:41 +0100, Richard Sandiford wrote:
>>>
>>> gcc/
>>> * Makefile.in (TARGET_DEF): Add target-insns.def.
>>> (.PRECIOUS, simple_rtl_generated_h): Add insn-target-def.h.
>>> (build/gentarget-def.o): New rule.
>>> (genprogrtl): Add target-def.
>>> * target-insns.def, gentarget-def.c: New files.
>>> * target.def: Add targetm.have_* and targetm.gen_* hooks,
>>> based on the contents of target-insns.def.
>>> * defaults.h (HAVE_simple_return, gen_simple_return): Delete.
>>> (HAVE_return, gen_return): Delete.
>>> * target-def.h: Include insn-target-def.h.
>>> * cfgrtl.c (force_nonfallthru_and_redirect): Use targetm interface
>>> instead of direct calls. Rely on them to do the appropriate assertions.
>>> * function.c (gen_return_pattern): Likewise. Return an rtx_insn *.
>>> (convert_jumps_to_returns): Use targetm interface instead of
>>> direct calls.
>>> (thread_prologue_and_epilogue_insns): Likewise.
>>> * reorg.c (find_end_label, dbr_schedule): Likewise.
>>> * shrink-wrap.h (SHRINK_WRAPPING_ENABLED): Likewise.
>>> * shrink-wrap.c (convert_to_simple_return): Likewise.
>>> (try_shrink_wrapping): Use SHRINK_WRAPPING_ENABLED.
>>
>> The patch breaks bootstrap on ppc64le. During libgcc configuration:
>
> Yeah, sorry about that. The problem is due to confusion about whether
> the generator should emit a barrier or not. After the final instruction
> in a sequence, it should always be up to the caller to insert any
> necessary barriers, just like it is for define_insns, and for define_expands
> implemented in C++ with DONE.
>
> I'm testing the following patch.
Committed after bootstrapping & regression-testing on x86_64-linux-gnu
and testing that Anton's testcase worked on ppc64le. (The emit-rtl.c
part seemed obvious given the generator changes.)
Sorry again for the breakage.
Thanks,
Richard
> gcc/
> * rtl.h (emit): Add an optional boolean parameter to control
> whether barriers are emitted.
> * emit-rtl.c (emit): Likewise.
> * gensupport.c (get_emit_function): Return null rather than "emit".
> * genemit.c (gen_emit_seq): Handle the null return value.
> Don't emit barriers after the final instruction in the sequence.
> * gentarget-def.c (main): Don't emit barriers after the instruction.
>
> Index: gcc/rtl.h
> ===================================================================
> --- gcc/rtl.h 2015-06-26 09:09:10.128602409 +0100
> +++ gcc/rtl.h 2015-06-26 09:09:10.316600233 +0100
> @@ -3494,7 +3494,7 @@ extern void add_insn (rtx_insn *);
> extern void add_insn_before (rtx, rtx, basic_block);
> extern void add_insn_after (rtx, rtx, basic_block);
> extern void remove_insn (rtx);
> -extern rtx_insn *emit (rtx);
> +extern rtx_insn *emit (rtx, bool = true);
> extern void emit_insn_at_entry (rtx);
> extern rtx gen_lowpart_SUBREG (machine_mode, rtx);
> extern rtx gen_const_mem (machine_mode, rtx);
> Index: gcc/emit-rtl.c
> ===================================================================
> --- gcc/emit-rtl.c 2015-06-26 09:09:10.124602455 +0100
> +++ gcc/emit-rtl.c 2015-06-26 09:09:10.316600233 +0100
> @@ -5303,11 +5303,14 @@ set_dst_reg_note (rtx insn, enum reg_not
> return NULL_RTX;
> }
> \f
> -/* Emit the rtl pattern X as an appropriate kind of insn.
> +/* Emit the rtl pattern X as an appropriate kind of insn. Also emit a
> + following barrier if the instruction needs one and if ALLOW_BARRIER_P
> + is true.
> +
> If X is a label, it is simply added into the insn chain. */
>
> rtx_insn *
> -emit (rtx x)
> +emit (rtx x, bool allow_barrier_p)
> {
> enum rtx_code code = classify_insn (x);
>
> @@ -5320,7 +5323,8 @@ emit (rtx x)
> case JUMP_INSN:
> {
> rtx_insn *insn = emit_jump_insn (x);
> - if (any_uncondjump_p (insn) || GET_CODE (x) == RETURN)
> + if (allow_barrier_p
> + && (any_uncondjump_p (insn) || GET_CODE (x) == RETURN))
> return emit_barrier ();
> return insn;
> }
> Index: gcc/gensupport.c
> ===================================================================
> --- gcc/gensupport.c 2015-06-26 09:09:10.124602455 +0100
> +++ gcc/gensupport.c 2015-06-26 09:09:10.316600233 +0100
> @@ -2983,7 +2983,9 @@ get_pattern_stats (struct pattern_stats
> stats->max_scratch_opno)) + 1;
> }
>
> -/* Return the emit_* function that should be used for pattern X. */
> +/* Return the emit_* function that should be used for pattern X, or NULL
> + if we can't pick a particular type at compile time and should instead
> + fall back to "emit". */
>
> const char *
> get_emit_function (rtx x)
> @@ -3000,7 +3002,7 @@ get_emit_function (rtx x)
> return "emit_jump_insn";
>
> case UNKNOWN:
> - return "emit";
> + return NULL;
>
> default:
> gcc_unreachable ();
> Index: gcc/genemit.c
> ===================================================================
> --- gcc/genemit.c 2015-06-26 09:09:10.124602455 +0100
> +++ gcc/genemit.c 2015-06-26 09:09:10.316600233 +0100
> @@ -277,12 +277,22 @@ gen_emit_seq (rtvec vec, char *used)
> {
> for (int i = 0, len = GET_NUM_ELEM (vec); i < len; ++i)
> {
> + bool last_p = (i == len - 1);
> rtx next = RTVEC_ELT (vec, i);
> - printf (" %s (", get_emit_function (next));
> - gen_exp (next, DEFINE_EXPAND, used);
> - printf (");\n");
> - if (needs_barrier_p (next))
> - printf (" emit_barrier ();");
> + if (const char *name = get_emit_function (next))
> + {
> + printf (" %s (", name);
> + gen_exp (next, DEFINE_EXPAND, used);
> + printf (");\n");
> + if (!last_p && needs_barrier_p (next))
> + printf (" emit_barrier ();");
> + }
> + else
> + {
> + printf (" emit (");
> + gen_exp (next, DEFINE_EXPAND, used);
> + printf (", %s);\n", last_p ? "false" : "true");
> + }
> }
> }
> \f
> Index: gcc/gentarget-def.c
> ===================================================================
> --- gcc/gentarget-def.c 2015-06-26 09:09:10.124602455 +0100
> +++ gcc/gentarget-def.c 2015-06-26 09:09:10.316600233 +0100
> @@ -245,7 +245,7 @@ main (int argc, char **argv)
> printf (" if (rtx_insn *insn = dyn_cast <rtx_insn *> (x))\n");
> printf (" return insn;\n");
> printf (" start_sequence ();\n");
> - printf (" emit (x);\n");
> + printf (" emit (x, false);\n");
> printf (" rtx_insn *res = get_insns ();\n");
> printf (" end_sequence ();\n");
> printf (" return res;\n");
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-06-23 18:42 Add .def file for public target instructions Richard Sandiford
` (2 preceding siblings ...)
2015-06-26 7:42 ` Markus Trippelsdorf
@ 2015-07-01 9:39 ` Trevor Saunders
2015-07-01 9:53 ` Richard Biener
3 siblings, 1 reply; 15+ messages in thread
From: Trevor Saunders @ 2015-07-01 9:39 UTC (permalink / raw)
To: gcc-patches, Mikhail Maltsev, rdsandiford
On Tue, Jun 23, 2015 at 07:41:42PM +0100, Richard Sandiford wrote:
> [A fair bit later than promised, sorry...]
>
> Mikhail posted a patch to make genflags generate the default HAVE_foo
> and gen_foo definitions that have recently been added to defaults.h:
>
> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>
> I agree it'd be a good idea to generate this kind of thing automatically,
> but I think we should take the opportunity to move the interface to the
> target structure. I.e.:
>
> HAVE_foo -> targetm.have_foo ()
> gen_foo -> targetm.gen_foo ()
>
> This should move us closer to the pipedream goal of supporting multiple
> targets at once. It should also mean that only the target code depends
> on insn-flags.h.
using targetm. certainly seems like an improvement. I wonder if it
would be faster to stick this data on a per function object. I think
that would mean you could compute what insns are available once when the
function is created and afterwards all checks would only needed to be
reading computed values.
Trev
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-07-01 9:39 ` Trevor Saunders
@ 2015-07-01 9:53 ` Richard Biener
2015-07-01 10:14 ` Richard Sandiford
0 siblings, 1 reply; 15+ messages in thread
From: Richard Biener @ 2015-07-01 9:53 UTC (permalink / raw)
To: Trevor Saunders; +Cc: GCC Patches, Mikhail Maltsev, Richard Sandiford
On Wed, Jul 1, 2015 at 11:39 AM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
> On Tue, Jun 23, 2015 at 07:41:42PM +0100, Richard Sandiford wrote:
>> [A fair bit later than promised, sorry...]
>>
>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>> and gen_foo definitions that have recently been added to defaults.h:
>>
>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>
>> I agree it'd be a good idea to generate this kind of thing automatically,
>> but I think we should take the opportunity to move the interface to the
>> target structure. I.e.:
>>
>> HAVE_foo -> targetm.have_foo ()
>> gen_foo -> targetm.gen_foo ()
>>
>> This should move us closer to the pipedream goal of supporting multiple
>> targets at once. It should also mean that only the target code depends
>> on insn-flags.h.
>
> using targetm. certainly seems like an improvement. I wonder if it
> would be faster to stick this data on a per function object. I think
> that would mean you could compute what insns are available once when the
> function is created and afterwards all checks would only needed to be
> reading computed values.
I think the memory cost of this is prohibitive.
Richard.
> Trev
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-07-01 9:53 ` Richard Biener
@ 2015-07-01 10:14 ` Richard Sandiford
2015-07-01 10:18 ` Richard Sandiford
0 siblings, 1 reply; 15+ messages in thread
From: Richard Sandiford @ 2015-07-01 10:14 UTC (permalink / raw)
To: Richard Biener; +Cc: Trevor Saunders, GCC Patches, Mikhail Maltsev
Richard Biener <richard.guenther@gmail.com> writes:
> On Wed, Jul 1, 2015 at 11:39 AM, Trevor Saunders <tbsaunde@tbsaunde.org> wrote:
>> On Tue, Jun 23, 2015 at 07:41:42PM +0100, Richard Sandiford wrote:
>>> [A fair bit later than promised, sorry...]
>>>
>>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>>> and gen_foo definitions that have recently been added to defaults.h:
>>>
>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>>
>>> I agree it'd be a good idea to generate this kind of thing automatically,
>>> but I think we should take the opportunity to move the interface to the
>>> target structure. I.e.:
>>>
>>> HAVE_foo -> targetm.have_foo ()
>>> gen_foo -> targetm.gen_foo ()
>>>
>>> This should move us closer to the pipedream goal of supporting multiple
>>> targets at once. It should also mean that only the target code depends
>>> on insn-flags.h.
>>
>> using targetm. certainly seems like an improvement. I wonder if it
>> would be faster to stick this data on a per function object. I think
>> that would mean you could compute what insns are available once when the
>> function is created and afterwards all checks would only needed to be
>> reading computed values.
>
> I think the memory cost of this is prohibitive.
Yeah. I did wonder about having optabs-like caching in some
target_globals structure, but I don't think it's really worth it.
In practice we only tend to call the have_*() functions when we're
trying to generate something (unlike optabs), and so any saving would
be dwarfed by the cost of generating whatever rtx we go on to create.
Thanks,
Richard
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Add .def file for public target instructions
2015-07-01 10:14 ` Richard Sandiford
@ 2015-07-01 10:18 ` Richard Sandiford
0 siblings, 0 replies; 15+ messages in thread
From: Richard Sandiford @ 2015-07-01 10:18 UTC (permalink / raw)
To: Richard Biener; +Cc: Trevor Saunders, GCC Patches, Mikhail Maltsev
Richard Sandiford <richard.sandiford@arm.com> writes:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Wed, Jul 1, 2015 at 11:39 AM, Trevor Saunders
>> <tbsaunde@tbsaunde.org> wrote:
>>> On Tue, Jun 23, 2015 at 07:41:42PM +0100, Richard Sandiford wrote:
>>>> [A fair bit later than promised, sorry...]
>>>>
>>>> Mikhail posted a patch to make genflags generate the default HAVE_foo
>>>> and gen_foo definitions that have recently been added to defaults.h:
>>>>
>>>> https://gcc.gnu.org/ml/gcc-patches/2015-06/msg00723.html
>>>>
>>>> I agree it'd be a good idea to generate this kind of thing automatically,
>>>> but I think we should take the opportunity to move the interface to the
>>>> target structure. I.e.:
>>>>
>>>> HAVE_foo -> targetm.have_foo ()
>>>> gen_foo -> targetm.gen_foo ()
>>>>
>>>> This should move us closer to the pipedream goal of supporting multiple
>>>> targets at once. It should also mean that only the target code depends
>>>> on insn-flags.h.
>>>
>>> using targetm. certainly seems like an improvement. I wonder if it
>>> would be faster to stick this data on a per function object. I think
>>> that would mean you could compute what insns are available once when the
>>> function is created and afterwards all checks would only needed to be
>>> reading computed values.
>>
>> I think the memory cost of this is prohibitive.
>
> Yeah. I did wonder about having optabs-like caching in some
> target_globals structure, but I don't think it's really worth it.
> In practice we only tend to call the have_*() functions when we're
> trying to generate something (unlike optabs), and so any saving would
> be dwarfed by the cost of generating whatever rtx we go on to create.
...plus (forgot the main reason): the condition for things like return
and simple_return are only available after register allocation.
Thanks,
Richard
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-07-01 10:18 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-06-23 18:42 Add .def file for public target instructions Richard Sandiford
2015-06-24 6:36 ` Jeff Law
2015-06-25 20:11 ` H.J. Lu
2015-06-25 23:00 ` H.J. Lu
2015-06-25 23:37 ` Andrew Pinski
2015-06-25 23:55 ` H.J. Lu
2015-06-26 6:33 ` H.J. Lu
2015-06-26 8:50 ` Richard Sandiford
2015-06-26 7:42 ` Markus Trippelsdorf
2015-06-26 8:45 ` Richard Sandiford
2015-06-26 10:15 ` Richard Sandiford
2015-07-01 9:39 ` Trevor Saunders
2015-07-01 9:53 ` Richard Biener
2015-07-01 10:14 ` Richard Sandiford
2015-07-01 10:18 ` Richard Sandiford
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).