Re: [PATCH 9/9] cse.c selftests

[Jeff Law <law at redhat dot com>]

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.



> ---
>  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.


> +
> +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.


 +
> +  /* 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?