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
