Re: Register numbers in RTL dumps (was Re: [PATCH 0/9] RFC: selftests based on RTL dumps)

[David Malcolm]

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)

[Bernd Schmidt]

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)

[Jeff Law]

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)

[Bernd Schmidt]

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)

[David Malcolm]

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

[Jeff Law]

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

[Bernd Schmidt]

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

[David Malcolm]

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

[David Malcolm]

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

[Jeff Law]

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

[Jeff Law]

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

[Bernd Schmidt]

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

[Richard Biener]

On Tue, Sep 13, 2016 at 10:37 PM, Jeff Law  wrote:
> 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

[Jeff Law]

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

[Bernd Schmidt]

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

[David Malcolm]

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

[Bernd Schmidt]

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

[David Malcolm]

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