On Fri, 2016-09-16 at 14:26 -0600, Jeff Law wrote:
> On 09/08/2016 06:30 PM, David Malcolm wrote:
> > This patch uses rtl_dump_test to start building out a test suite
> > for cse.
> >
> > I attempted to create a reproducer for PR 71779; however I'm not
> > yet
> > able to replicate the bogus cse reported there via the test case.
> >
> > gcc/ChangeLog:
> > * cse.c: Include selftest.h and selftest-rtl.h.
> > (selftest::test_simple_cse): New function.
> > (selftest::test_pr71779): New function.
> > (selftest::cse_c_tests): New function.
> > * selftest-run-tests.c (selftest::run_tests): Call
> > selftest::cse_c_tests.
> > * selftest.h (selftest::cse_c_tests): New decl.
> So as I look at this, the first thing that comes to mind is whether
> or
> not to refine the dump output.
>
> There's a lot of useless crap in there that really only helps folks
> that
> sitting inside a debugger dumping hunks of RTL (I'm thinking
> specifically about the pointers back to tree nodes for DECLs). Those
> addresses are useless in other contexts.
>
> When possible I don't think we want the tests to be target specific.
> Hmm, I'm probably about to argue for Bernd's work... The 71779
> testcase
> is a great example -- it uses high/lo_sum. Not all targets support
> that
> -- as long as we don't try to recognize those insns we're likely OK,
> but
> that seems fragile long term. Having an idealized target means we
> can
> ignore much of these issues.
An alternative would be to pick a specific target for each test.
> > ---
> > gcc/cse.c | 109
> > +++++++++++++++++++++++++++++++++++++++++++++++
> > gcc/selftest-run-tests.c | 1 +
> > gcc/selftest.h | 1 +
> > 3 files changed, 111 insertions(+)
> >
> > diff --git a/gcc/cse.c b/gcc/cse.c
> > index 0bfd7ff..f4f06fe 100644
> > --- a/gcc/cse.c
> > +++ b/gcc/cse.c
> > @@ -41,6 +41,8 @@ along with GCC; see the file COPYING3. If not
> > see
> > #include "tree-pass.h"
> > #include "dbgcnt.h"
> > #include "rtl-iter.h"
> > +#include "selftest.h"
> > +#include "selftest-rtl.h"
> >
> > #ifndef LOAD_EXTEND_OP
> > #define LOAD_EXTEND_OP(M) UNKNOWN
> > @@ -7773,3 +7775,110 @@ make_pass_cse_after_global_opts
> > (gcc::context *ctxt)
> > {
> > return new pass_cse_after_global_opts (ctxt);
> > }
> > +
> > +#if CHECKING_P
> > +
> > +namespace selftest {
> > +
> > +/* Selftests for CSE. */
> > +
> > +/* Simple test of eliminating a redundant (reg + 1) computation
> > + i.e. that:
> > + r101 = r100 + 1;
> > + r102 = r100 + 1; <<< common subexpression
> > + *r103 = r101 * r102;
> > + can be CSE-ed to:
> > + r101 = r100 + 1;
> > + r102 = r101; <<< replaced
> > + *r103 = r101 * r102;
> > + by cse_main. */
> So I'm real curious, what happens if you run this RTL selftest under
> valgrind? I have the sneaking suspicion that we'll start doing some
> uninitialized memory reads.
I'm seeing various leaks (an htab within linemap_init for all of the
line_table fixtures), but no uninitialized reads.
> > +
> > +static void
> > +test_simple_cse ()
> > +{
> > + /* Only run this tests for i386. */
> > +#ifndef I386_OPTS_H
> > + return;
> > +#endif
> > +
> > + const char *input_dump
> > + = (/* "r101 = r100 + 1;" */
> > + "(insn 1 0 2 2 (set (reg:SI 101)\n"
> > + " (plus:SI (reg:SI 100)\n"
> > + " (const_int 1 [0x1]))) -1
> > (nil))\n"
> > + /* "r102 = r100 + 1;" */
> > + "(insn 2 1 3 2 (set (reg:SI 102)\n"
> > + " (plus:SI (reg:SI 100)\n"
> > + " (const_int 1 [0x1]))) -1
> > (nil))\n"
> > + /* "*r103 = r101 * r102;" */
> > + "(insn 3 2 0 2 (set (mem:SI (reg:SI 103) [1 i+0 S4 A32])\n"
> > + " (mult:SI (reg:SI 101) (reg:SI 102))) -1
> > (nil))\n"
> > + );
> I don't think we need to comment each RTL insn for those which are so
> obvious. It just adds to the visual clutter.
This may have been more for my benefit; I'm still relatively new to RTL
:)
> +
> > + /* Dump taken from comment 2 of PR 71779, of
> > + "...the relevant memory access coming out of expand"
> > + with basic block IDs added, and prev/next insns set to
> > + 0 at ends. */
> > + const char *input_dump
> > + = (";; MEM[(struct isl_obj *)&obj1] = &isl_obj_map_vtable;\n"
> > + "(insn 1045 0 1046 2 (set (reg:SI 480)\n"
> > + " (high:SI (symbol_ref:SI (\"isl_obj_map_vtable\")
> > [flags 0xc0] <var_decl 0x7fa0363ea240 isl_obj_map_vtable>)))
> > y.c:12702 -1\n"
> > + " (nil))\n"
> > + "(insn 1046 1045 1047 2 (set (reg/f:SI 479)\n"
> > + " (lo_sum:SI (reg:SI 480)\n"
> > + " (symbol_ref:SI (\"isl_obj_map_vtable\") [flags
> > 0xc0] <var_decl 0x7fa0363ea240 isl_obj_map_vtable>))) y.c:12702
> > -1\n"
> > + " (expr_list:REG_EQUAL (symbol_ref:SI
> > (\"isl_obj_map_vtable\") [flags 0xc0] <var_decl 0x7fa0363ea240
> > isl_obj_map_vtable>)\n"
> > + " (nil)))\n"
> > + "(insn 1047 1046 1048 2 (set (reg:DI 481)\n"
> > + " (subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1\n"
> > + " (nil))\n"
> > + "(insn 1048 1047 1049 2 (set (zero_extract:DI (reg/v:DI 191
> > [ obj1D.17368 ])\n"
> > + " (const_int 32 [0x20])\n"
> > + " (const_int 0 [0]))\n"
> > + " (reg:DI 481)) y.c:12702 -1\n"
> > + " (nil))\n"
> So looking at this just makes my head hurt. Escaping, lots of
> quotes,
> unnecessary things in the dump, etc. The question I would have is
> why
> not have a file with the dump and read the file?
(nods)
Seems like I need to add a mechanism for telling the selftests which
directory to load the tests relative to.
