Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)
On Tue, 2016-09-20 at 09:20 -0600, Jeff Law wrote: > On 09/20/2016 08:34 AM, Bernd Schmidt wrote: > > On 09/20/2016 04:32 PM, David Malcolm wrote: > > > > > > To summarize so far: you want every pseudo to have its regno > > > dumped > > > with a 'p' prefix, and renumber them on dump (and then on load). > > > OK. > > > > Renumbering is not helpful because it interferes with the view you > > have > > in the debugger. So, IMO just a prefix, and maybe > Yea, I guess it does since we want the numbers in the dump to be the > same that we used to access the arrays. So prefixing in the dump > with > adjustment of the number in the reader. To check I understand: am I right in thinking you want: (A) the numbers in the dump to be unmodified when dumping, so that we can easily look up values in arrays without confusion, and (B) regnums in the dump gain a 'p' prefix for values >= FIRST_PSEUDO_REGISTER, so that humans and parsers can easily see when the regs are pseudos, and that (C) the parser will detect if a 'p'-prefixed regno actually has the same number as a hard reg (which can happen e.g. when a .md file changes, or when sharing .rtl dumps between targets), and remap the values on load accordingly ? (in which case we do need the regno_remapper class, or something like it) > > > > > (reg/f:DI v1 virtual-stack-vars) > > > > this. > Doesn't the same apply to the number of virtual stack regs? Those > are > in the same array as pseudos. So ISTM we prefix in the dump, but do > adjustment of the number in the reader? Presumably we could use "v" rather than "p" as the prefix for the first 5 pseudos (up to LAST_VIRTUAL_REGISTER), doing any adjustment at load time, rather than at dump time. So the above example would look like: (reg/f:DI v82 virtual-stack-vars) i.e. the 82 for x86_64's virtual-stack-vars would be prefixed with a 'v', and the loader would adjust it to be the current target's value for VIRTUAL_STACK_VARS_REGNUM. Do you like the idea of prefixing regnums of hardregs with 'h'? (so that all regnos get a one-char prefix) e.g. (reg/i:SI h0 ax) (reg/i:SF h21 xmm0) Dave
Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)
On 09/20/2016 05:20 PM, Jeff Law wrote: On 09/20/2016 08:34 AM, Bernd Schmidt wrote: (reg/f:DI v1 virtual-stack-vars) this. Doesn't the same apply to the number of virtual stack regs? Those are in the same array as pseudos. So ISTM we prefix in the dump, but do adjustment of the number in the reader? I meant the virtual-stack-vars name. But maybe we're already doing that, I can't remember. Bernd
Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)
On 09/20/2016 08:34 AM, Bernd Schmidt wrote: On 09/20/2016 04:32 PM, David Malcolm wrote: To summarize so far: you want every pseudo to have its regno dumped with a 'p' prefix, and renumber them on dump (and then on load). OK. Renumbering is not helpful because it interferes with the view you have in the debugger. So, IMO just a prefix, and maybe Yea, I guess it does since we want the numbers in the dump to be the same that we used to access the arrays. So prefixing in the dump with adjustment of the number in the reader. (reg/f:DI v1 virtual-stack-vars) this. Doesn't the same apply to the number of virtual stack regs? Those are in the same array as pseudos. So ISTM we prefix in the dump, but do adjustment of the number in the reader? jeff
Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)
On 09/20/2016 04:32 PM, David Malcolm wrote: To summarize so far: you want every pseudo to have its regno dumped with a 'p' prefix, and renumber them on dump (and then on load). OK. Renumbering is not helpful because it interferes with the view you have in the debugger. So, IMO just a prefix, and maybe (reg/f:DI v1 virtual-stack-vars) this. Bernd
Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)
On Mon, 2016-09-19 at 11:35 -0600, Jeff Law wrote: > On 09/16/2016 03:27 PM, David Malcolm wrote: > > > We should also twiddle how we represent registers in the dumps. > > > Identifying hard regs by name (so we can map back to a hard reg > > > if > > > the > > > hard regs change), identifying pseudos by number that isn't > > > affected > > > if > > > the hard register set changes ie, p0, p1, p2, p3 where the number > > > is > > > REGNO (x) - FIRST_PSEUDO_REGISTER. identifying the virtual > > > registers, > > > etc. > > > > Good idea. > > > > I don't know if you saw this yet, but the patch has logic from > > renumbering pseudos on load (see class regno_remapper), but dumping > > them as p0, p1 etc and reloading them as such seems much easier for > > everyone. > And just an FYI, I think we should do this unconditionally in our > dumps. To summarize so far: you want every pseudo to have its regno dumped with a 'p' prefix, and renumber them on dump (and then on load). OK. When dumping regnos, would you want another distinction between virtuals and non-virtuals in the dump? For example, given that LAST_VIRTUAL_POINTER_REGISTER is ((FIRST_VIRTUAL_REGISTER) + 4), this could mean: v0, v1, ..., v4 for the virtual regnos and, for pseudos that aren't virtual regnos: p0, p1, ... or p5, p6, ... depending on whether we want to start numbering the pseudos at p0 for LAST_VIRTUAL_REGISTER + 1, or whether we regard v0 as the first pseduo, and hence p5 is the first non-virtual pseudo. FWIW I think starting at p5 is the better approach, given that: #define FIRST_VIRTUAL_REGISTER(FIRST_PSEUDO_REGISTER) Either way, this would give things like: (reg/f:DI v1 virtual-stack-vars) (reg:DI p78) In a similar vein, how about adding a "h" prefix for the hard regnos? That way every regno would have a one-char prefix telling you what kind of reg it is (and you can directly see whether or not you need to offset the number by FIRST_PSEUDO_REGISTER to get at the "real" regno). This would give things (for x86_64) like: (reg/i:SI h0 ax) (reg/i:SF h21 xmm0) > > > > > The key being rather than put a ton of smarts/hacks in a reader, > > > we > > > should work to have the RTL writer give us something more useful. > > > That > > > may mean simple changes to the output, or some conditional > > > changes > > > (like > > > not emitting the INSN_CODE or its name). > > > > That would make the reader a lot less grim; it has a lot of warts > > for > > dealing with all the special cases in the current output format. > That's the idea. > > > > But maybe it is useful to be able to deal with the current output > > format. For example, I was able to look at PR 71779 and find some > > fragmentary RTL dumps there (comment #2) and use them. I *did* > > need to > > edit them a little, so maybe it's OK to require a little editing > > (especially with older dumps... to what extent has the format > > changed > > over the years?) > It's changed, but not in radical ways. > > I think the case we want to cater to is dumping something from the > current compiler rather than picking up some crusty RTL and creating > a > testcase from that. By "current" I explicitly want the ability to > refine the output to make the reader easier to implement. > > Jeff
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/16/2016 03:27 PM, David Malcolm wrote: We should also twiddle how we represent registers in the dumps. Identifying hard regs by name (so we can map back to a hard reg if the hard regs change), identifying pseudos by number that isn't affected if the hard register set changes ie, p0, p1, p2, p3 where the number is REGNO (x) - FIRST_PSEUDO_REGISTER. identifying the virtual registers, etc. Good idea. I don't know if you saw this yet, but the patch has logic from renumbering pseudos on load (see class regno_remapper), but dumping them as p0, p1 etc and reloading them as such seems much easier for everyone. And just an FYI, I think we should do this unconditionally in our dumps. The key being rather than put a ton of smarts/hacks in a reader, we should work to have the RTL writer give us something more useful. That may mean simple changes to the output, or some conditional changes (like not emitting the INSN_CODE or its name). That would make the reader a lot less grim; it has a lot of warts for dealing with all the special cases in the current output format. That's the idea. But maybe it is useful to be able to deal with the current output format. For example, I was able to look at PR 71779 and find some fragmentary RTL dumps there (comment #2) and use them. I *did* need to edit them a little, so maybe it's OK to require a little editing (especially with older dumps... to what extent has the format changed over the years?) It's changed, but not in radical ways. I think the case we want to cater to is dumping something from the current compiler rather than picking up some crusty RTL and creating a testcase from that. By "current" I explicitly want the ability to refine the output to make the reader easier to implement. Jeff
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/16/2016 11:12 PM, David Malcolm wrote: There are some interrelated questions here: (a) where do the dumps live? (string fragments embedded in the source file vs external files) (b) -fself-test vs DejaGnu tests that use a real frontend. In the latter case, is the frontend "rtl1", or an extension of "cc1" with an "__RTL" marker? I think a rtl1 frontend that gets run from a specific subdirectory in testsuite/ is probably the best option. For (a), I'd like to do support both (in that it's clear we need support for external files, but it seems trivial to support embedding). If we're dealing with small snippets, I'm not sure embedding them as strings is really that much better than building them up with gen_ functions. I'm sure there's room for rtl selftests living inside the compiler like that, but anything larger probably ought to live outside. Bernd
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On Fri, 2016-09-16 at 14:05 -0600, Jeff Law wrote: > On 09/13/2016 05:15 AM, Bernd Schmidt wrote: > > > > > > Note that the loader now resets INSN_CODE to -1, regardless of > > > the > > > actual code passed in, to force re-recognition, and to isolate > > > the > > > dumps somewhat from changes to the .md files. So although the > > > above > > > says insn 641 and 642 (for some snapshot of the aarch64 md file), > > > it > > > gets reset to -1. > > > > Best to find out a way to avoid including it in the strings then, > > to > > avoid confusion. > We should also twiddle how we represent registers in the dumps. > Identifying hard regs by name (so we can map back to a hard reg if > the > hard regs change), identifying pseudos by number that isn't affected > if > the hard register set changes ie, p0, p1, p2, p3 where the number is > REGNO (x) - FIRST_PSEUDO_REGISTER. identifying the virtual registers, > etc. Good idea. I don't know if you saw this yet, but the patch has logic from renumbering pseudos on load (see class regno_remapper), but dumping them as p0, p1 etc and reloading them as such seems much easier for everyone. > The key being rather than put a ton of smarts/hacks in a reader, we > should work to have the RTL writer give us something more useful. > That > may mean simple changes to the output, or some conditional changes > (like > not emitting the INSN_CODE or its name). That would make the reader a lot less grim; it has a lot of warts for dealing with all the special cases in the current output format. But maybe it is useful to be able to deal with the current output format. For example, I was able to look at PR 71779 and find some fragmentary RTL dumps there (comment #2) and use them. I *did* need to edit them a little, so maybe it's OK to require a little editing (especially with older dumps... to what extent has the format changed over the years?)
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On Fri, 2016-09-16 at 14:00 -0600, Jeff Law wrote: > On 09/14/2016 02:37 AM, Richard Biener wrote: > > > > Note that while the "snippets" may largely work (not sure how many > > you tried to come up with) I see the issue that a lot of RTL "unit > > tests" would need some trees set up, like to properly form > > MEM_EXPRs > > or REG_DECL or even SYMBOL_REFs. So I fear that the scope of > > unit-tests we can implement with the proposed scheme is very > > limited > > (you may also need other stuff setup, like alias analysis or parts > > of > > IRA or cost analysis parts). So I agree a separate testing backend > > is a good way to make unit-testing more stable on the target side > > we > > also need a way to provide input on some of the global state that > > is > > currently set up by frontends. > Agreed across the board. I think we're still early in the learning > phase on this stuff. I shudder when I think about the amount of > state > that we depend on, but which is not represented in the RTL dumps. > > But I do think there are some things we can test for in an RTL self > testing framework and that having one would be a significant step > forward. > > So I think we have two big questions. > > First, does David's work have value as a way to directly test pieces > of > the RTL pipeline without having to generate all the RTL bits by hand. > I > think the answer is yes. > > Second, will David's work help identify internal state that is not > expressed in the RTL dumps or poor modularity (ie, cases like trees > embedded within the RTL structures). I think the answer to this is > yes > as well. > > Third, is a framework like Bernd's useful as well and can it be mated > with David's work. And I think the answer is yes & yes as well with > the > result being more useful than either Bernd or David's work in > isolation. As far as I understand Bernd's suggestion there seem to be two parts to it: (a) a kind of virtual target, which can act in different ways depending on the needs of a test case. e.g. dynamically select 32 bit vs 64 bit, does it have a CCmode, how many stages is the pipeline etc etc. (b) a separate build of the "gcc" subdir, configured to use (a). These seem like laudable goal, but I see it as orthogonal to my patch kit. It'd also be a massive expansion of scope. Also, re (a) any given test is likely to be tested against a specific target. That could be a real target, or a particular setting of a virtual target. The tests in my patch kit have largely gated on the specific targets I was testing with. So is Bernd's suggestion a prerequisite for my work, or can my work stand alone from it? > > > > But my biggest worry is with putting unit-tests into cc1 itself -- > > even more so with RTL unit tests of this kind than with all the > > other > > ones we have. We'll quickly have 99% of a source file comprised of > > RTL unit tests rather than source (and cc1 object size as well). > > Hardly something we want to have (not even mentioning bootstrap > > time > > issues). > I can live revisiting this -- I always expected we would after we > started building out the framework. There are some interrelated questions here: (a) where do the dumps live? (string fragments embedded in the source file vs external files) (b) -fself-test vs DejaGnu tests that use a real frontend. In the latter case, is the frontend "rtl1", or an extension of "cc1" with an "__RTL" marker? For (a), I'd like to do support both (in that it's clear we need support for external files, but it seems trivial to support embedding). I've worked with both approaches in the past, and both are useful (a two-liner may make sense to live "inline", as they get larger you'd want them in a separate file). For (b), I'd like to do both: if nothing else, the loader itself needs selftests. Plus selftests allow for tests that are more fine-grained than just the level of one optimization pass. But I'd anticipate most of them being external. For selftests that load external files, there's a slight wart - how are they accessed? There needs to be some way to tell it which directory to look in. Also, what happens when build != host? I have some followup patches that both build out an actual frontend on top of the loader, and extend cc1, but there's some ugly diagnostic and linemap issues to resolve. > > > > Yes, putting the unit-tests in source files makes us not require > > exporting an interface to the parts we are testing. But that's > > about > > the only advantage I can see. You didn't show that it isn't > > possible > > to put the small test you were writing into a RTL-frontendish test > > which starts compiling the function with the test with the pass you > > are about to unit-test. > The other advantage is tests which use the internal APIs are easy to > identify/fix when an internal API is changed. > > Jeff >
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/13/2016 05:15 AM, Bernd Schmidt wrote: Note that the loader now resets INSN_CODE to -1, regardless of the actual code passed in, to force re-recognition, and to isolate the dumps somewhat from changes to the .md files. So although the above says insn 641 and 642 (for some snapshot of the aarch64 md file), it gets reset to -1. Best to find out a way to avoid including it in the strings then, to avoid confusion. We should also twiddle how we represent registers in the dumps. Identifying hard regs by name (so we can map back to a hard reg if the hard regs change), identifying pseudos by number that isn't affected if the hard register set changes ie, p0, p1, p2, p3 where the number is REGNO (x) - FIRST_PSEUDO_REGISTER. identifying the virtual registers, etc. The key being rather than put a ton of smarts/hacks in a reader, we should work to have the RTL writer give us something more useful. That may mean simple changes to the output, or some conditional changes (like not emitting the INSN_CODE or its name). jeff
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/14/2016 02:37 AM, Richard Biener wrote: Note that while the "snippets" may largely work (not sure how many you tried to come up with) I see the issue that a lot of RTL "unit tests" would need some trees set up, like to properly form MEM_EXPRs or REG_DECL or even SYMBOL_REFs. So I fear that the scope of unit-tests we can implement with the proposed scheme is very limited (you may also need other stuff setup, like alias analysis or parts of IRA or cost analysis parts). So I agree a separate testing backend is a good way to make unit-testing more stable on the target side we also need a way to provide input on some of the global state that is currently set up by frontends. Agreed across the board. I think we're still early in the learning phase on this stuff. I shudder when I think about the amount of state that we depend on, but which is not represented in the RTL dumps. But I do think there are some things we can test for in an RTL self testing framework and that having one would be a significant step forward. So I think we have two big questions. First, does David's work have value as a way to directly test pieces of the RTL pipeline without having to generate all the RTL bits by hand. I think the answer is yes. Second, will David's work help identify internal state that is not expressed in the RTL dumps or poor modularity (ie, cases like trees embedded within the RTL structures). I think the answer to this is yes as well. Third, is a framework like Bernd's useful as well and can it be mated with David's work. And I think the answer is yes & yes as well with the result being more useful than either Bernd or David's work in isolation. But my biggest worry is with putting unit-tests into cc1 itself -- even more so with RTL unit tests of this kind than with all the other ones we have. We'll quickly have 99% of a source file comprised of RTL unit tests rather than source (and cc1 object size as well). Hardly something we want to have (not even mentioning bootstrap time issues). I can live revisiting this -- I always expected we would after we started building out the framework. Yes, putting the unit-tests in source files makes us not require exporting an interface to the parts we are testing. But that's about the only advantage I can see. You didn't show that it isn't possible to put the small test you were writing into a RTL-frontendish test which starts compiling the function with the test with the pass you are about to unit-test. The other advantage is tests which use the internal APIs are easy to identify/fix when an internal API is changed. Jeff
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/13/2016 01:15 PM, Bernd Schmidt wrote: On 09/12/2016 08:57 PM, David Malcolm wrote: I'm not sure I follow - this sounds like a dedicated target for selftesting. Would it be a separate configuration i.e. something like: ../src/configure --target=rtl-selftest or somesuch? The way I imagine it working, the top-level Makefile would create a selftest-gcc/ subdirectory, and run a configure line much like the above inside it. It would live independently of the real compiler we're building in gcc/. The Makefile goop would look something like this, adapted from the old accel-gcc stuff we had on gomp-4_0-branch. Autogenerated files are omitted, run autogen/autoconf. It's gated on an --enable switch. While it would be nice to include it in every build, it would probably not be worth the extra build time until such a time when we've built up a fairly large library of tests. I picked ia64-elf as a stand-in for now. I guess if we decide to go down this path we can start trying to decide what we want the actual backend to look like. Bernd Index: Makefile.def === --- Makefile.def (revision 240029) +++ Makefile.def (working copy) @@ -47,6 +47,9 @@ host_modules= { module= flex; no_check_c host_modules= { module= gas; bootstrap=true; }; host_modules= { module= gcc; bootstrap=true; extra_make_flags="$(EXTRA_GCC_FLAGS)"; }; +host_modules= { module= test-gcc; + module_srcdir=gcc; + no_install= true; }; host_modules= { module= gmp; lib_path=.libs; bootstrap=true; // Work around in-tree gmp configure bug with missing flex. extra_configure_flags='--disable-shared LEX="touch lex.yy.c"'; Index: Makefile.tpl === --- Makefile.tpl (revision 240029) +++ Makefile.tpl (working copy) @@ -1047,6 +1047,11 @@ configure-[+prefix+][+module+]: [+ IF bo $(SHELL) $(srcdir)/mkinstalldirs [+subdir+]/[+module+]; \ [+exports+] [+extra_exports+] \ echo Configuring in [+subdir+]/[+module+]; \ + [+ IF (= (get "module") "test-gcc") +] \ + this_target="ia64-elf"; \ + [+ ELSE +] \ + this_target="[+target_alias+]"; \ + [+ ENDIF +] \ cd "[+subdir+]/[+module+]" || exit 1; \ case $(srcdir) in \ /* | [A-Za-z]:[\\/]*) topdir=$(srcdir) ;; \ @@ -1059,7 +1064,7 @@ configure-[+prefix+][+module+]: [+ IF bo $$s/$$module_srcdir/configure \ --srcdir=$${topdir}/$$module_srcdir \ [+args+] --build=${build_alias} --host=[+host_alias+] \ - --target=[+target_alias+] [+extra_configure_flags+] \ + --target=$${this_target} [+extra_configure_flags+] \ || exit 1 @endif [+prefix+][+module+] Index: configure.ac === --- configure.ac (revision 240029) +++ configure.ac (working copy) @@ -140,7 +140,7 @@ host_libs="intl libiberty opcodes bfd re # binutils, gas and ld appear in that order because it makes sense to run # "make check" in that particular order. # If --enable-gold is used, "gold" may replace "ld". -host_tools="texinfo flex bison binutils gas ld fixincludes gcc cgen sid sim gdb gprof etc expect dejagnu m4 utils guile fastjar gnattools libcc1 gotools" +host_tools="texinfo flex bison binutils gas ld fixincludes test-gcc gcc cgen sid sim gdb gprof etc expect dejagnu m4 utils guile fastjar gnattools libcc1 gotools" # libgcj represents the runtime libraries only used by gcj. libgcj="target-libffi \ @@ -305,6 +305,15 @@ AC_ARG_ENABLE(offload-targets, fi ], [enable_offload_targets=]) +AC_ARG_ENABLE(gcc-rtl-test, +[AS_HELP_STRING([[--enable-gcc-rtl-test[=ARG]]], + [build the rtl selftest compiler])], +ENABLE_RTL_SELFTEST=$enableval, +ENABLE_RTL_SELFTEST=no) +case "${ENABLE_RTL_SELFTEST}" in + no) skipdirs="${skipdirs} test-gcc" ;; +esac + # Handle --enable-gold, --enable-ld. # --disable-gold [--enable-ld] # Build only ld. Default option. @@ -2246,7 +2255,15 @@ done configdirs_all="$configdirs" configdirs= for i in ${configdirs_all} ; do - if test -f ${srcdir}/$i/configure ; then + case $i in +test-gcc) + confsrcdir=gcc + ;; +*) + confsrcdir=$i + ;; + esac + if test -f ${srcdir}/${confsrcdir}/configure ; then configdirs="${configdirs} $i" fi done
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On Tue, Sep 13, 2016 at 10:37 PM, Jeff Lawwrote: > On 09/12/2016 12:57 PM, David Malcolm wrote: >> >> >> I'm not sure I follow - this sounds like a dedicated target for >> selftesting. > > That's exactly what it is. We'd essentially put in knobs so that we could > control the different things we need to. For example, if we wanted to test > a particular problem with promotions of arguments, we can do that. If we > wanted to test how we handled a secondary address reload to implemenet a > GP<->FP register copy through memory, we can control all the parameters for > that too. And so on. > > It's Bernd's idea and I think it has a lot of merit and I think it's largely > complementary to what you're doing. Note that while the "snippets" may largely work (not sure how many you tried to come up with) I see the issue that a lot of RTL "unit tests" would need some trees set up, like to properly form MEM_EXPRs or REG_DECL or even SYMBOL_REFs. So I fear that the scope of unit-tests we can implement with the proposed scheme is very limited (you may also need other stuff setup, like alias analysis or parts of IRA or cost analysis parts). So I agree a separate testing backend is a good way to make unit-testing more stable on the target side we also need a way to provide input on some of the global state that is currently set up by frontends. But my biggest worry is with putting unit-tests into cc1 itself -- even more so with RTL unit tests of this kind than with all the other ones we have. We'll quickly have 99% of a source file comprised of RTL unit tests rather than source (and cc1 object size as well). Hardly something we want to have (not even mentioning bootstrap time issues). Yes, putting the unit-tests in source files makes us not require exporting an interface to the parts we are testing. But that's about the only advantage I can see. You didn't show that it isn't possible to put the small test you were writing into a RTL-frontendish test which starts compiling the function with the test with the pass you are about to unit-test. Richard. > jeff >
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/12/2016 12:57 PM, David Malcolm wrote: I'm not sure I follow - this sounds like a dedicated target for selftesting. That's exactly what it is. We'd essentially put in knobs so that we could control the different things we need to. For example, if we wanted to test a particular problem with promotions of arguments, we can do that. If we wanted to test how we handled a secondary address reload to implemenet a GP<->FP register copy through memory, we can control all the parameters for that too. And so on. It's Bernd's idea and I think it has a lot of merit and I think it's largely complementary to what you're doing. jeff
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On 09/12/2016 08:57 PM, David Malcolm wrote: I'm not sure I follow - this sounds like a dedicated target for selftesting. Would it be a separate configuration i.e. something like: ../src/configure --target=rtl-selftest or somesuch? The way I imagine it working, the top-level Makefile would create a selftest-gcc/ subdirectory, and run a configure line much like the above inside it. It would live independently of the real compiler we're building in gcc/. That's not something I'm deciding, it needs a broader consensus. But I feel pretty strongly that this is how things should be organized. + const char *input_dump += ("(insn 8 0 9 2 (set (reg:DI 78)\n" + "(lshiftrt:DI (reg:DI 76)\n" + "(const_int 32 [0x20])))\n" + "../../src/gcc/testsuite/gcc.dg/asr_div1.c:14\n" + "641 {*aarch64_lshr_sisd_or_int_di3}\n" + " (expr_list:REG_DEAD (reg:DI 76)\n" + "(nil)))\n" + "(insn 9 8 0 2 (set (reg:SI 79)\n" + "(ashiftrt:SI (subreg:SI (reg:DI 78) 0)\n" + "(const_int 3 [0x3])))\n" + "../../src/gcc/testsuite/gcc.dg/asr_div1.c:14\n" + "642 {*aarch64_ashr_sisd_or_int_si3}\n" + " (expr_list:REG_DEAD (reg:DI 78)\n" + "(nil)))\n"); I can sort of see the desire to just copy dumps into this, but this strikes me as really ugly. Especially if we end up using real targets this hard-codes not just pattern structure but also pattern names, which I think is too great a burden on target maintainers. Note that the loader now resets INSN_CODE to -1, regardless of the actual code passed in, to force re-recognition, and to isolate the dumps somewhat from changes to the .md files. So although the above says insn 641 and 642 (for some snapshot of the aarch64 md file), it gets reset to -1. Best to find out a way to avoid including it in the strings then, to avoid confusion. Bernd
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
On Mon, 2016-09-12 at 16:10 +0200, Bernd Schmidt wrote: > In general, it's functionality that I would very much like to have > (although maybe it's less useful these days now that the rtl side is > fairly static). I think there's not much sense looking too deeply at > the > individual patches yet; we need to first figure out what we would > really > like this to look like in the end. > > > These tests are very target-specific and were developed mostly for > > target==x86_64, and a little for target==aarch64. > > I put clauses like this in the various test functions, which is a > > kludge: > > > > /* Only run these tests for i386. */ > > #ifndef I386_OPTS_H > > return; > > #endif > > > > Is there a better way to express this condition? (I guess I could > > add a selftest::target_is_x86_p () predicate). > > My preferred solution would still be a separate selftest backend, > which > could be built as a cross-compiler once in a separate top-level > directory. That way we have control over it, and maintainers of > actual > targets don't need to fear breaking selftests when they make changes > to > their ports. The downside would primarily be the time to build it. I'm not sure I follow - this sounds like a dedicated target for selftesting. Would it be a separate configuration i.e. something like: ../src/configure --target=rtl-selftest or somesuch? > > + const char *input_dump > > += ("(insn 8 0 9 2 (set (reg:DI 78)\n" > > + "(lshiftrt:DI (reg:DI 76)\n" > > + "(const_int 32 [0x20])))\n" > > + "../../src/gcc/testsuite/gcc.dg/asr_div1.c:14\n" > > + "641 {*aarch64_lshr_sisd_or_int_di3}\n" > > + " (expr_list:REG_DEAD (reg:DI 76)\n" > > + "(nil)))\n" > > + "(insn 9 8 0 2 (set (reg:SI 79)\n" > > + "(ashiftrt:SI (subreg:SI (reg:DI 78) 0)\n" > > + "(const_int 3 [0x3])))\n" > > + "../../src/gcc/testsuite/gcc.dg/asr_div1.c:14\n" > > + "642 {*aarch64_ashr_sisd_or_int_si3}\n" > > + " (expr_list:REG_DEAD (reg:DI 78)\n" > > + "(nil)))\n"); > > I can sort of see the desire to just copy dumps into this, but > this strikes me as really ugly. Especially if we end up using real > targets this hard-codes not just pattern structure but also pattern > names, which I think is too great a burden on target maintainers. Note that the loader now resets INSN_CODE to -1, regardless of the actual code passed in, to force re-recognition, and to isolate the dumps somewhat from changes to the .md files. So although the above says insn 641 and 642 (for some snapshot of the aarch64 md file), it gets reset to -1. > It's also not unheard of for the insn structure to change a bit; I > remember a change which swapped location and pattern (I think). > > There's also a fairly large amount of visual clutter here, such as > the > input filenames. > > Maybe there's room for a tool to take input dumps and convert them to > something more readable, or maybe to a sequence of gen_/emit_ > function > calls? (such a tool could use the loader class, and e.g. strip out the location information).
Re: [PATCH 0/9] RFC: selftests based on RTL dumps
In general, it's functionality that I would very much like to have (although maybe it's less useful these days now that the rtl side is fairly static). I think there's not much sense looking too deeply at the individual patches yet; we need to first figure out what we would really like this to look like in the end. These tests are very target-specific and were developed mostly for target==x86_64, and a little for target==aarch64. I put clauses like this in the various test functions, which is a kludge: /* Only run these tests for i386. */ #ifndef I386_OPTS_H return; #endif Is there a better way to express this condition? (I guess I could add a selftest::target_is_x86_p () predicate). My preferred solution would still be a separate selftest backend, which could be built as a cross-compiler once in a separate top-level directory. That way we have control over it, and maintainers of actual targets don't need to fear breaking selftests when they make changes to their ports. The downside would primarily be the time to build it. + const char *input_dump += ("(insn 8 0 9 2 (set (reg:DI 78)\n" + "(lshiftrt:DI (reg:DI 76)\n" + "(const_int 32 [0x20])))\n" + "../../src/gcc/testsuite/gcc.dg/asr_div1.c:14\n" + "641 {*aarch64_lshr_sisd_or_int_di3}\n" + " (expr_list:REG_DEAD (reg:DI 76)\n" + "(nil)))\n" + "(insn 9 8 0 2 (set (reg:SI 79)\n" + "(ashiftrt:SI (subreg:SI (reg:DI 78) 0)\n" + "(const_int 3 [0x3])))\n" + "../../src/gcc/testsuite/gcc.dg/asr_div1.c:14\n" + "642 {*aarch64_ashr_sisd_or_int_si3}\n" + " (expr_list:REG_DEAD (reg:DI 78)\n" + "(nil)))\n"); I can sort of see the desire to just copy dumps into this, but this strikes me as really ugly. Especially if we end up using real targets this hard-codes not just pattern structure but also pattern names, which I think is too great a burden on target maintainers. It's also not unheard of for the insn structure to change a bit; I remember a change which swapped location and pattern (I think). There's also a fairly large amount of visual clutter here, such as the input filenames. Maybe there's room for a tool to take input dumps and convert them to something more readable, or maybe to a sequence of gen_/emit_ function calls? Bernd
[PATCH 0/9] RFC: selftests based on RTL dumps
The current selftest code is adequate for testing individual instructions, but most interesting passes have logic involving the interaction of multiple instructions, or require a CFG and function to be runnable. In theory we could write selftests by programatically constructing a function and CFG "by hand" via API calls, but this is awkward, tedious, and the resulting code is unnatural. Examples can be seen in function-tests.c; that file constructs trivial 3-block functions, and is pushing the limits of what I'd want to write "by hand". This patch kit provides a more natural way to write RTL selftests, by providing a parser for RTL dumps (or fragments of RTL dumps). You can copy and paste fragments of RTL dump into the source for a pass and then have selftests that run part of the pass on that dump, asserting that desired outcomes occur. This is an updated and rewritten version of the RTL frontend work I posted in May (c.f. "[PATCH 0/4] RFC: RTL frontend" https://gcc.gnu.org/ml/gcc-patches/2016-05/msg00352.html). In that patch kit, I introduced an rtl1 frontend, capable of parsing RTL dumps, and running just one RTL pass on them, in the hope of better testing individual RTL passes. This patch kit takes a somewhat different approach: it provides the infrastructure for parsing RTL dumps, but doesn't wire it up as a frontend. Instead, it just provides a set of classes for use when writing selftests. It would be possible to build out an "rtl1" frontend as a followup to this kit. The advantage of this approach is that it's possible to run and test passes at finer granularity: for example, rather than being limited to testing all of, say, pass "final", we can also write tests that run just final_scan_insn on individual rtx_insn *, and verify that the expected output is emitted. Tests can be written at various different levels, testing how the components of a pass handle fragments of an insn, how they handle entire insns, basic blocks, and so on up to running a whole pass on a whole function. The disadvantage is that changing a selftest requires recompilation of cc1 (though that drawback affects selftests in general). An overview of the kit follows; patches 6-9 are probably the most interesting, as they show example of the kinds of selftest that can be written using this approach: * Patch 1 tidies up some global state in .md parsing, wrapping it up in an rtx_reader class. * Patches 2-4 add some selftest support code. * Patch 5 introduces a function_reader subclass of patch 1's rtx_reader, capable of parsing RTL function dumps (or fragments thereof), and generalizes things so that rtx parsing can be run from the host, rather than just at build time. * Patch 6 uses the infrastructure to write a dataflow selftest. It's a trivial example, but hopefully shows how more interesting selftests could be written. (it's much easier to write a selftest if a similar one already exists) * Patch 7 does the same, but for combine.c. * Patch 8 does the same, but for final.c, showing examples of both assembling an entire function, and of assembling individual rtx_insn * (to exercise the .md files and asm output code) * Patch 9 does the same, but for cse.c. I attempted to create a selftest that directly reproduces PR 71779, though I haven't yet been able to to reproduce the issue (just load the insns and run cse on them). These tests are very target-specific and were developed mostly for target==x86_64, and a little for target==aarch64. I put clauses like this in the various test functions, which is a kludge: /* Only run these tests for i386. */ #ifndef I386_OPTS_H return; #endif Is there a better way to express this condition? (I guess I could add a selftest::target_is_x86_p () predicate). Posting for comment (doesn't fully bootstrap yet due to a few stray warnings). Patches 1-4 could probably be applicable even without the rest of the kit. Thoughts? David Malcolm (9): Introduce class rtx_reader Add selftest::read_file selftest.h: add temp_override fixture Expose forcibly_ggc_collect and run it after all selftests Introduce class function_reader df selftests combine.c selftests final.c selftests cse.c selftests gcc/Makefile.in |5 + gcc/cfgexpand.c |7 +- gcc/combine.c| 155 ++ gcc/cse.c| 109 + gcc/df-core.c| 77 +++ gcc/emit-rtl.c | 70 ++- gcc/emit-rtl.h |2 + gcc/errors.c | 23 +- gcc/errors.h | 13 + gcc/final.c | 271 +++ gcc/function-tests.c |2 +- gcc/function.c | 41 +- gcc/function.h |4 +- gcc/genconstants.c |3 +- gcc/genenums.c |3 +- gcc/genmddeps.c |3 +- gcc/genpreds.c |9 +- gcc/gensupport.c | 29 +- gcc/gensupport.h |6 +- gcc/ggc-tests.c