| Message ID | 1480644013-3660-3-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Dec 01, 2016 at 09:00:11PM -0500, David Malcolm wrote: > This patch adds more selftests for class function_reader, where > the dumps to be read contain aarch64-specific features. > > In an earlier version of the patch kit, these were handled using > #ifndef GCC_AARCH64_H to conditionalize running them. > This version instead runs them via a target hook for running > target-specific selftests, thus putting them within aarch64.c. I'm probably missing something obvious here. This looks OK, but can we have a comment somewhere near the code as to what this test is actually testing? Is it just that x0 is correctly identfied as the return register? Thanks, James > > gcc/ChangeLog: > * config/aarch64/aarch64.c: Include selftest.h and > selftest-rtl.h. > (selftest::aarch64_test_loading_full_dump): New function. > (selftest::aarch64_run_selftests): New function. > (TARGET_RUN_TARGET_SELFTESTS): Wire it up to > selftest::aarch64_run_selftests. > > gcc/testsuite/ChangeLog: > * selftests/aarch64: New subdirectory. > * selftests/aarch64/times-two.rtl: New file. > --- > gcc/config/aarch64/aarch64.c | 49 +++++++++++++++++++++++++++ > gcc/testsuite/selftests/aarch64/times-two.rtl | 36 ++++++++++++++++++++ > 2 files changed, 85 insertions(+) > create mode 100644 gcc/testsuite/selftests/aarch64/times-two.rtl > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index bd97c5b..d19bee3 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -64,6 +64,8 @@ > #include "sched-int.h" > #include "target-globals.h" > #include "common/common-target.h" > +#include "selftest.h" > +#include "selftest-rtl.h" > > /* This file should be included last. */ > #include "target-def.h" > @@ -14168,6 +14170,48 @@ aarch64_optab_supported_p (int op, machine_mode mode1, machine_mode, > } > } > > +/* Target-specific selftests. */ > + > +#if CHECKING_P > + > +namespace selftest { > + > +/* Selftest for the RTL loader. This test is target-specific and thus > + here since the dump contains target-specific hard reg names. > + Verify that the RTL loader copes with a dump from print_rtx_function. */ > + > +static void > +aarch64_test_loading_full_dump () > +{ > + rtl_dump_test t (SELFTEST_LOCATION, locate_file ("aarch64/times-two.rtl")); > + > + ASSERT_STREQ ("times_two", IDENTIFIER_POINTER (DECL_NAME (cfun->decl))); > + > + rtx_insn *insn_1 = get_insn_by_uid (1); > + ASSERT_EQ (NOTE, GET_CODE (insn_1)); > + > + rtx_insn *insn_15 = get_insn_by_uid (15); > + ASSERT_EQ (INSN, GET_CODE (insn_15)); > + ASSERT_EQ (USE, GET_CODE (PATTERN (insn_15))); > + > + /* Verify crtl->return_rtx. */ > + ASSERT_EQ (REG, GET_CODE (crtl->return_rtx)); > + ASSERT_EQ (0, REGNO (crtl->return_rtx)); > + ASSERT_EQ (SImode, GET_MODE (crtl->return_rtx)); > +} > + > +/* Run all target-specific selftests. */ > + > +static void > +aarch64_run_selftests (void) > +{ > + aarch64_test_loading_full_dump (); > +} > + > +} // namespace selftest > + > +#endif /* #if CHECKING_P */ > + > #undef TARGET_ADDRESS_COST > #define TARGET_ADDRESS_COST aarch64_address_cost > > @@ -14502,6 +14546,11 @@ aarch64_optab_supported_p (int op, machine_mode mode1, machine_mode, > #undef TARGET_OMIT_STRUCT_RETURN_REG > #define TARGET_OMIT_STRUCT_RETURN_REG true > > +#if CHECKING_P > +#undef TARGET_RUN_TARGET_SELFTESTS > +#define TARGET_RUN_TARGET_SELFTESTS selftest::aarch64_run_selftests > +#endif /* #if CHECKING_P */ > + > struct gcc_target targetm = TARGET_INITIALIZER; > > #include "gt-aarch64.h" > diff --git a/gcc/testsuite/selftests/aarch64/times-two.rtl b/gcc/testsuite/selftests/aarch64/times-two.rtl > new file mode 100644 > index 0000000..dbce67e > --- /dev/null > +++ b/gcc/testsuite/selftests/aarch64/times-two.rtl > @@ -0,0 +1,36 @@ > +(function "times_two" > + (insn-chain > + (cnote 1 NOTE_INSN_DELETED) > + (block 2 > + (edge-from entry (flags "FALLTHRU")) > + (cnote 4 [bb 2] NOTE_INSN_BASIC_BLOCK) > + (cinsn 2 (set (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars) > + (const_int -4)) [1 i+0 S4 A32]) > + (reg:SI x0 [ i ])) "../../src/times-two.c":2 > + (nil)) > + (cnote 3 NOTE_INSN_FUNCTION_BEG) > + (cinsn 6 (set (reg:SI %2) > + (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars) > + (const_int -4)) [1 i+0 S4 A32])) "../../src/times-two.c":3 > + (nil)) > + (cinsn 7 (set (reg:SI %0 [ _2 ]) > + (ashift:SI (reg:SI %2) > + (const_int 1))) "../../src/times-two.c":3 > + (nil)) > + (cinsn 10 (set (reg:SI %1 [ <retval> ]) > + (reg:SI %0 [ _2 ])) "../../src/times-two.c":3 > + (nil)) > + (cinsn 14 (set (reg/i:SI x0) > + (reg:SI %1 [ <retval> ])) "../../src/times-two.c":4 > + (nil)) > + (cinsn 15 (use (reg/i:SI x0)) "../../src/times-two.c":4 > + (nil)) > + (edge-to exit (flags "FALLTHRU")) > + ) ;; block 2 > + ) ;; insn-chain > + (crtl > + (return_rtx > + (reg/i:SI x0) > + ) ;; return_rtx > + ) ;; crtl > +) ;; function "times_two" > -- > 1.8.5.3 >
On Tue, 2016-12-06 at 17:22 +0000, James Greenhalgh wrote: > On Thu, Dec 01, 2016 at 09:00:11PM -0500, David Malcolm wrote: > > This patch adds more selftests for class function_reader, where > > the dumps to be read contain aarch64-specific features. > > > > In an earlier version of the patch kit, these were handled using > > #ifndef GCC_AARCH64_H to conditionalize running them. > > This version instead runs them via a target hook for running > > target-specific selftests, thus putting them within aarch64.c. > > I'm probably missing something obvious here. > > This looks OK, but can we have a comment somewhere near the code as > to > what this test is actually testing? > > Is it just that x0 is correctly identfied as the return register? The original point of the test was to have an integration test of loading a real dump from print_rtx_function, to verify that the loader can actually load it. The dump contains a target-specific item, and thus the test needs to be made target-specific (I did one of these for x86_64, and one for aarch64, which are the two targets that I've done the most testing of the patch kit on). Looking over it now, yeah, it's not a great test (but hopefully not a bad one either). It does verify that "x0" is correctly parsed, so it is giving some test coverage for lookup_reg_by_dump_name's handling of hard regs (in patch 8a in the kit). I also picked a couple of insns to verify (one outside of a bb, the other inside of a bb). Currently the comment reads: /* Selftest for the RTL loader. This test is target-specific and here since the dump contains target-specific hard reg names. Verify that the RTL loader copes with a dump from print_rtx_function. */ Would you be OK with the test if it read: /* Selftest for the RTL loader. Verify that the RTL loader copes with a dump from print_rtx_function. This is essentially just a test that class function_reader can handle a real dump, but it also verifies that lookup_reg_by_dump_name correctly handles hard regs. The presence of hard reg names in the dump means that the test is target-specific, hence it is in this file. */ or somesuch? Alternatively, I can drop this patch. Thanks Dave > Thanks, > James > > > > > gcc/ChangeLog: > > * config/aarch64/aarch64.c: Include selftest.h and > > selftest-rtl.h. > > (selftest::aarch64_test_loading_full_dump): New function. > > (selftest::aarch64_run_selftests): New function. > > (TARGET_RUN_TARGET_SELFTESTS): Wire it up to > > selftest::aarch64_run_selftests. > > > > gcc/testsuite/ChangeLog: > > * selftests/aarch64: New subdirectory. > > * selftests/aarch64/times-two.rtl: New file. > > --- > > gcc/config/aarch64/aarch64.c | 49 > > +++++++++++++++++++++++++++ > > gcc/testsuite/selftests/aarch64/times-two.rtl | 36 > > ++++++++++++++++++++ > > 2 files changed, 85 insertions(+) > > create mode 100644 gcc/testsuite/selftests/aarch64/times-two.rtl > > > > diff --git a/gcc/config/aarch64/aarch64.c > > b/gcc/config/aarch64/aarch64.c > > index bd97c5b..d19bee3 100644 > > --- a/gcc/config/aarch64/aarch64.c > > +++ b/gcc/config/aarch64/aarch64.c > > @@ -64,6 +64,8 @@ > > #include "sched-int.h" > > #include "target-globals.h" > > #include "common/common-target.h" > > +#include "selftest.h" > > +#include "selftest-rtl.h" > > > > /* This file should be included last. */ > > #include "target-def.h" > > @@ -14168,6 +14170,48 @@ aarch64_optab_supported_p (int op, > > machine_mode mode1, machine_mode, > > } > > } > > > > +/* Target-specific selftests. */ > > + > > +#if CHECKING_P > > + > > +namespace selftest { > > + > > +/* Selftest for the RTL loader. This test is target-specific and > > thus > > + here since the dump contains target-specific hard reg names. > > + Verify that the RTL loader copes with a dump from > > print_rtx_function. */ > > + > > +static void > > +aarch64_test_loading_full_dump () > > +{ > > + rtl_dump_test t (SELFTEST_LOCATION, locate_file ("aarch64/times > > -two.rtl")); > > + > > + ASSERT_STREQ ("times_two", IDENTIFIER_POINTER (DECL_NAME (cfun > > ->decl))); > > + > > + rtx_insn *insn_1 = get_insn_by_uid (1); > > + ASSERT_EQ (NOTE, GET_CODE (insn_1)); > > + > > + rtx_insn *insn_15 = get_insn_by_uid (15); > > + ASSERT_EQ (INSN, GET_CODE (insn_15)); > > + ASSERT_EQ (USE, GET_CODE (PATTERN (insn_15))); > > + > > + /* Verify crtl->return_rtx. */ > > + ASSERT_EQ (REG, GET_CODE (crtl->return_rtx)); > > + ASSERT_EQ (0, REGNO (crtl->return_rtx)); > > + ASSERT_EQ (SImode, GET_MODE (crtl->return_rtx)); > > +} > > + > > +/* Run all target-specific selftests. */ > > + > > +static void > > +aarch64_run_selftests (void) > > +{ > > + aarch64_test_loading_full_dump (); > > +} > > + > > +} // namespace selftest > > + > > +#endif /* #if CHECKING_P */ > > + > > #undef TARGET_ADDRESS_COST > > #define TARGET_ADDRESS_COST aarch64_address_cost > > > > @@ -14502,6 +14546,11 @@ aarch64_optab_supported_p (int op, > > machine_mode mode1, machine_mode, > > #undef TARGET_OMIT_STRUCT_RETURN_REG > > #define TARGET_OMIT_STRUCT_RETURN_REG true > > > > +#if CHECKING_P > > +#undef TARGET_RUN_TARGET_SELFTESTS > > +#define TARGET_RUN_TARGET_SELFTESTS > > selftest::aarch64_run_selftests > > +#endif /* #if CHECKING_P */ > > + > > struct gcc_target targetm = TARGET_INITIALIZER; > > > > #include "gt-aarch64.h" > > diff --git a/gcc/testsuite/selftests/aarch64/times-two.rtl > > b/gcc/testsuite/selftests/aarch64/times-two.rtl > > new file mode 100644 > > index 0000000..dbce67e > > --- /dev/null > > +++ b/gcc/testsuite/selftests/aarch64/times-two.rtl > > @@ -0,0 +1,36 @@ > > +(function "times_two" > > + (insn-chain > > + (cnote 1 NOTE_INSN_DELETED) > > + (block 2 > > + (edge-from entry (flags "FALLTHRU")) > > + (cnote 4 [bb 2] NOTE_INSN_BASIC_BLOCK) > > + (cinsn 2 (set (mem/c:SI (plus:DI (reg/f:DI virtual-stack > > -vars) > > + (const_int -4)) [1 i+0 S4 A32]) > > + (reg:SI x0 [ i ])) "../../src/times-two.c":2 > > + (nil)) > > + (cnote 3 NOTE_INSN_FUNCTION_BEG) > > + (cinsn 6 (set (reg:SI %2) > > + (mem/c:SI (plus:DI (reg/f:DI virtual-stack > > -vars) > > + (const_int -4)) [1 i+0 S4 A32])) > > "../../src/times-two.c":3 > > + (nil)) > > + (cinsn 7 (set (reg:SI %0 [ _2 ]) > > + (ashift:SI (reg:SI %2) > > + (const_int 1))) "../../src/times-two.c":3 > > + (nil)) > > + (cinsn 10 (set (reg:SI %1 [ <retval> ]) > > + (reg:SI %0 [ _2 ])) "../../src/times-two.c":3 > > + (nil)) > > + (cinsn 14 (set (reg/i:SI x0) > > + (reg:SI %1 [ <retval> ])) "../../src/times > > -two.c":4 > > + (nil)) > > + (cinsn 15 (use (reg/i:SI x0)) "../../src/times-two.c":4 > > + (nil)) > > + (edge-to exit (flags "FALLTHRU")) > > + ) ;; block 2 > > + ) ;; insn-chain > > + (crtl > > + (return_rtx > > + (reg/i:SI x0) > > + ) ;; return_rtx > > + ) ;; crtl > > +) ;; function "times_two"
On Tue, Dec 06, 2016 at 02:38:45PM -0500, David Malcolm wrote: > On Tue, 2016-12-06 at 17:22 +0000, James Greenhalgh wrote: > > On Thu, Dec 01, 2016 at 09:00:11PM -0500, David Malcolm wrote: > > > This patch adds more selftests for class function_reader, where > > > the dumps to be read contain aarch64-specific features. > > > > > > In an earlier version of the patch kit, these were handled using > > > #ifndef GCC_AARCH64_H to conditionalize running them. > > > This version instead runs them via a target hook for running > > > target-specific selftests, thus putting them within aarch64.c. > > > > I'm probably missing something obvious here. > > > > This looks OK, but can we have a comment somewhere near the code as > > to > > what this test is actually testing? > > > > Is it just that x0 is correctly identfied as the return register? > > The original point of the test was to have an integration test of > loading a real dump from print_rtx_function, to verify that the loader > can actually load it. > > The dump contains a target-specific item, and thus the test needs to be > made target-specific (I did one of these for x86_64, and one for > aarch64, which are the two targets that I've done the most testing of > the patch kit on). > > Looking over it now, yeah, it's not a great test (but hopefully not a > bad one either). > > It does verify that "x0" is correctly parsed, so it is giving some test > coverage for lookup_reg_by_dump_name's handling of hard regs (in patch > 8a in the kit). I also picked a couple of insns to verify (one outside > of a bb, the other inside of a bb). > > Currently the comment reads: > > /* Selftest for the RTL loader. This test is target-specific and > here since the dump contains target-specific hard reg names. > Verify that the RTL loader copes with a dump from > print_rtx_function. */ > > Would you be OK with the test if it read: > > /* Selftest for the RTL loader. > Verify that the RTL loader copes with a dump from > print_rtx_function. This is essentially just a test that class > function_reader can handle a real dump, but it also verifies > that lookup_reg_by_dump_name correctly handles hard regs. > The presence of hard reg names in the dump means that the test is > target-specific, hence it is in this file. */ > > or somesuch? That looks fine to me. Thanks for the more detailed explanation. > Alternatively, I can drop this patch. No, this is good, and I'm happy to approve it - I just haven't followed the rest of the patch series so it wasn't clear to me from the syntax what made this target specific. With your explanation I now understand. This is OK for trunk. Thanks, James
diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c index bd97c5b..d19bee3 100644 --- a/gcc/config/aarch64/aarch64.c +++ b/gcc/config/aarch64/aarch64.c @@ -64,6 +64,8 @@ #include "sched-int.h" #include "target-globals.h" #include "common/common-target.h" +#include "selftest.h" +#include "selftest-rtl.h" /* This file should be included last. */ #include "target-def.h" @@ -14168,6 +14170,48 @@ aarch64_optab_supported_p (int op, machine_mode mode1, machine_mode, } } +/* Target-specific selftests. */ + +#if CHECKING_P + +namespace selftest { + +/* Selftest for the RTL loader. This test is target-specific and thus + here since the dump contains target-specific hard reg names. + Verify that the RTL loader copes with a dump from print_rtx_function. */ + +static void +aarch64_test_loading_full_dump () +{ + rtl_dump_test t (SELFTEST_LOCATION, locate_file ("aarch64/times-two.rtl")); + + ASSERT_STREQ ("times_two", IDENTIFIER_POINTER (DECL_NAME (cfun->decl))); + + rtx_insn *insn_1 = get_insn_by_uid (1); + ASSERT_EQ (NOTE, GET_CODE (insn_1)); + + rtx_insn *insn_15 = get_insn_by_uid (15); + ASSERT_EQ (INSN, GET_CODE (insn_15)); + ASSERT_EQ (USE, GET_CODE (PATTERN (insn_15))); + + /* Verify crtl->return_rtx. */ + ASSERT_EQ (REG, GET_CODE (crtl->return_rtx)); + ASSERT_EQ (0, REGNO (crtl->return_rtx)); + ASSERT_EQ (SImode, GET_MODE (crtl->return_rtx)); +} + +/* Run all target-specific selftests. */ + +static void +aarch64_run_selftests (void) +{ + aarch64_test_loading_full_dump (); +} + +} // namespace selftest + +#endif /* #if CHECKING_P */ + #undef TARGET_ADDRESS_COST #define TARGET_ADDRESS_COST aarch64_address_cost @@ -14502,6 +14546,11 @@ aarch64_optab_supported_p (int op, machine_mode mode1, machine_mode, #undef TARGET_OMIT_STRUCT_RETURN_REG #define TARGET_OMIT_STRUCT_RETURN_REG true +#if CHECKING_P +#undef TARGET_RUN_TARGET_SELFTESTS +#define TARGET_RUN_TARGET_SELFTESTS selftest::aarch64_run_selftests +#endif /* #if CHECKING_P */ + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-aarch64.h" diff --git a/gcc/testsuite/selftests/aarch64/times-two.rtl b/gcc/testsuite/selftests/aarch64/times-two.rtl new file mode 100644 index 0000000..dbce67e --- /dev/null +++ b/gcc/testsuite/selftests/aarch64/times-two.rtl @@ -0,0 +1,36 @@ +(function "times_two" + (insn-chain + (cnote 1 NOTE_INSN_DELETED) + (block 2 + (edge-from entry (flags "FALLTHRU")) + (cnote 4 [bb 2] NOTE_INSN_BASIC_BLOCK) + (cinsn 2 (set (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars) + (const_int -4)) [1 i+0 S4 A32]) + (reg:SI x0 [ i ])) "../../src/times-two.c":2 + (nil)) + (cnote 3 NOTE_INSN_FUNCTION_BEG) + (cinsn 6 (set (reg:SI %2) + (mem/c:SI (plus:DI (reg/f:DI virtual-stack-vars) + (const_int -4)) [1 i+0 S4 A32])) "../../src/times-two.c":3 + (nil)) + (cinsn 7 (set (reg:SI %0 [ _2 ]) + (ashift:SI (reg:SI %2) + (const_int 1))) "../../src/times-two.c":3 + (nil)) + (cinsn 10 (set (reg:SI %1 [ <retval> ]) + (reg:SI %0 [ _2 ])) "../../src/times-two.c":3 + (nil)) + (cinsn 14 (set (reg/i:SI x0) + (reg:SI %1 [ <retval> ])) "../../src/times-two.c":4 + (nil)) + (cinsn 15 (use (reg/i:SI x0)) "../../src/times-two.c":4 + (nil)) + (edge-to exit (flags "FALLTHRU")) + ) ;; block 2 + ) ;; insn-chain + (crtl + (return_rtx + (reg/i:SI x0) + ) ;; return_rtx + ) ;; crtl +) ;; function "times_two"