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"
