* Re: [PATCH 9/9] cse.c selftests
@ 2016-09-09 13:32 Bernd Edlinger
2016-09-09 23:17 ` Jeff Law
0 siblings, 1 reply; 7+ messages in thread
From: Bernd Edlinger @ 2016-09-09 13:32 UTC (permalink / raw)
To: David Malcolm; +Cc: GCC Patches
[-- Attachment #1: Type: text/plain, Size: 3974 bytes --]
Hi David,
> 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.
Thanks, this is just awesome. I immediately had to try your patch.
The main reason for PR 71779 was that this
(insn 1047 1046 1048 (set (reg:DI 481)
(subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1
(nil))
got transformed into that:
(insn 1047 1046 1048 107 (set (reg/f:DI 481)
(subreg:DI (reg/f:SI 477) 0)) y.c:12702 50 {*movdi_aarch64}
(expr_list:REG_DEAD (reg/f:SI 479)
(nil)))
Note the "/f at reg 481. This together with the fact that -mabi=ilp32
makes Pmode != ptr_mode caused a cascade of different errors.
For the wrong transformation in combine to happen, we need
Pmode = DImode, ptr_mode=SImode, and REG_POINTER(x)
is true, but only REG_POINTER is really wrong.
First the good news. The unit test does actually work, but not only
on aarch even without -mabi=ilp32, but also on i386. All that's
missing is a check that we don't get a reg/f here:
test_pr71779 ()
{
/* Only run this tests for target==aarch64. */
#ifndef GCC_AARCH64_H
#ifndef I386_OPTS_H
return;
#endif
#endif
...
tem = cse_main (get_insns (), max_reg_num ());
ASSERT_EQ (0, tem);
ASSERT_FALSE (REG_POINTER (SET_DEST (PATTERN (get_insn_by_uid (1047)))));
}
Test fails on aarch64 and i386 if I revert the attached patch for PR 71779,
and passes again, if I apply the patch again. Excellent!
Attached for your reference, the original patch file from PR 71779.
/home/ed/gnu/gcc-build-aarch64/./gcc/xgcc -B/home/ed/gnu/gcc-build-aarch64/./gcc/ -xc -S -c /dev/null -fself-test
../../gcc-trunk/gcc/cse.c:7872: test_pr71779: FAIL: ASSERT_FALSE (((__extension__ ({ __typeof (((((PATTERN (get_insn_by_uid (1047)))->u.fld[0]).rt_rtx))) const _rtx = (((((PATTERN (get_insn_by_uid (1047)))->u.fld[0]).rt_rtx))); if (((enum rtx_code) (_rtx)->code) != REG) rtl_check_failed_flag ("REG_POINTER", _rtx, "../../gcc-trunk/gcc/cse.c", 7872, __FUNCTION__); _rtx; })->frame_related)))
In function 'test_1':
cc1: internal compiler error: in fail, at selftest.c:46
0x11bdd8e selftest::fail(selftest::location const&, char const*)
../../gcc-trunk/gcc/selftest.c:46
0x10944ec test_pr71779
../../gcc-trunk/gcc/cse.c:7872
0x10944ec selftest::cse_c_tests()
../../gcc-trunk/gcc/cse.c:7881
0x11631fe selftest::run_tests()
../../gcc-trunk/gcc/selftest-run-tests.c:68
0xb36242 toplev::run_self_tests()
../../gcc-trunk/gcc/toplev.c:2074
I noticed, that ASSERT_FALSE does expand the macros, which makes
the output a bit hard to read.
One minor thing, an unrelated test did fail before the cse test got executed on my aarch64,
so I just commented that part out:
/home/ed/gnu/gcc-build-aarch64/./gcc/xgcc -B/home/ed/gnu/gcc-build-aarch64/./gcc/ -xc -S -c /dev/null -fself-test
../../gcc-trunk/gcc/read-rtl-function.c:923: test_loading_dump_fragment_2: FAIL: ASSERT_TRUE ((((lhs)->frame_related)))
In function 'test_1':
cc1: internal compiler error: in fail, at selftest.c:46
0x11be8ae selftest::fail(selftest::location const&, char const*)
../../gcc-trunk/gcc/selftest.c:46
0x115577a test_loading_dump_fragment_2
../../gcc-trunk/gcc/read-rtl-function.c:923
0x1157cce selftest::read_rtl_function_c_tests()
../../gcc-trunk/gcc/read-rtl-function.c:1183
0x1163d14 selftest::run_tests()
../../gcc-trunk/gcc/selftest-run-tests.c:66
0xb36262 toplev::run_self_tests()
../../gcc-trunk/gcc/toplev.c:2074
the aarch64 was configured this way:
../gcc-trunk/configure --prefix=/home/ed/gnu/aarch64-unknown-elf --target=aarch64-unknown-elf --enable-languages=c --disable-shared --disable-threads --disable-libssp --disable-libgomp --disable-libquadmath --disable-libatomic
Maybe one last comment on your patch itself, could you please move
the unit test cases in extra unit test files, or even a self-test tree?
Thanks
Bernd.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-pr71779.diff --]
[-- Type: text/x-patch; name="patch-pr71779.diff", Size: 960 bytes --]
2016-08-04 Bernd Edlinger <bernd.edlinger@hotmail.de>
PR rtl-optimization/71779
* emit-rtl.c (set_reg_attrs_from_value): Only propagate REG_POINTER,
if the value was sign-extended according to POINTERS_EXTEND_UNSIGNED
or if it was truncated.
Index: gcc/emit-rtl.c
===================================================================
--- gcc/emit-rtl.c (revision 238915)
+++ gcc/emit-rtl.c (working copy)
@@ -1156,7 +1156,11 @@ set_reg_attrs_from_value (rtx reg, rtx x)
{
#if defined(POINTERS_EXTEND_UNSIGNED)
if (((GET_CODE (x) == SIGN_EXTEND && POINTERS_EXTEND_UNSIGNED)
- || (GET_CODE (x) != SIGN_EXTEND && ! POINTERS_EXTEND_UNSIGNED))
+ || (GET_CODE (x) == ZERO_EXTEND && ! POINTERS_EXTEND_UNSIGNED)
+ || (paradoxical_subreg_p (x)
+ && ! (SUBREG_PROMOTED_VAR_P (x)
+ && SUBREG_CHECK_PROMOTED_SIGN (x,
+ POINTERS_EXTEND_UNSIGNED))))
&& !targetm.have_ptr_extend ())
can_be_reg_pointer = false;
#endif
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 9/9] cse.c selftests
2016-09-09 13:32 [PATCH 9/9] cse.c selftests Bernd Edlinger
@ 2016-09-09 23:17 ` Jeff Law
2016-09-10 10:17 ` Bernd Edlinger
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Law @ 2016-09-09 23:17 UTC (permalink / raw)
To: Bernd Edlinger, David Malcolm; +Cc: GCC Patches
On 09/09/2016 07:28 AM, Bernd Edlinger wrote:
> Hi David,
>
>> 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.
>
> Thanks, this is just awesome. I immediately had to try your patch.
I'd pointed David at 71779 because it was such a good example of the
kind of thing we'd like to be able to avoid by using the self-testing
framework.
ie, someone says "hey, I don't think this code is needed anymore, let's
remove it" and hope nothing breaks. We do have the regression suite now
which helps considerably, but changes early in the optimization pipeline
may easily mask problems introduced much later in the pipeline.
With David's work, the goal is to be able to hand off reasonable hunks
of RTL to internal routines and see how they manipulate the RTL and
verify correctness. Essentially bypassing everything except the
routines in question that we want to test.
There's a lot of unknowns in this space and I'm hoping that David's work
will shed some light on how we can build a robust RTL test framework.
>
> The main reason for PR 71779 was that this
>
> (insn 1047 1046 1048 (set (reg:DI 481)
> (subreg:DI (reg/f:SI 479) 0)) y.c:12702 -1
> (nil))
>
> got transformed into that:
>
> (insn 1047 1046 1048 107 (set (reg/f:DI 481)
> (subreg:DI (reg/f:SI 477) 0)) y.c:12702 50 {*movdi_aarch64}
> (expr_list:REG_DEAD (reg/f:SI 479)
> (nil)))
>
> Note the "/f at reg 481. This together with the fact that -mabi=ilp32
> makes Pmode != ptr_mode caused a cascade of different errors.
>
> For the wrong transformation in combine to happen, we need
> Pmode = DImode, ptr_mode=SImode, and REG_POINTER(x)
> is true, but only REG_POINTER is really wrong.
>
> First the good news. The unit test does actually work, but not only
> on aarch even without -mabi=ilp32, but also on i386. All that's
> missing is a check that we don't get a reg/f here:
Sweet. This starts to touch on some of those unanswered questions --
while 71779 was visibly only failing on aarch64, can we build tests
which are ultimately target independent? I suspected that would be the
case for 71779, but whether or not that will hold in general remains to
be seen.
>
> test_pr71779 ()
> {
> /* Only run this tests for target==aarch64. */
> #ifndef GCC_AARCH64_H
> #ifndef I386_OPTS_H
> return;
> #endif
> #endif
> ...
> tem = cse_main (get_insns (), max_reg_num ());
> ASSERT_EQ (0, tem);
> ASSERT_FALSE (REG_POINTER (SET_DEST (PATTERN (get_insn_by_uid (1047)))));
> }
>
>
> One minor thing, an unrelated test did fail before the cse test got executed on my aarch64,
> so I just commented that part out:
>
>
> /home/ed/gnu/gcc-build-aarch64/./gcc/xgcc -B/home/ed/gnu/gcc-build-aarch64/./gcc/ -xc -S -c /dev/null -fself-test
> ../../gcc-trunk/gcc/read-rtl-function.c:923: test_loading_dump_fragment_2: FAIL: ASSERT_TRUE ((((lhs)->frame_related)))
> In function 'test_1':
> cc1: internal compiler error: in fail, at selftest.c:46
> 0x11be8ae selftest::fail(selftest::location const&, char const*)
> ../../gcc-trunk/gcc/selftest.c:46
> 0x115577a test_loading_dump_fragment_2
> ../../gcc-trunk/gcc/read-rtl-function.c:923
> 0x1157cce selftest::read_rtl_function_c_tests()
> ../../gcc-trunk/gcc/read-rtl-function.c:1183
> 0x1163d14 selftest::run_tests()
> ../../gcc-trunk/gcc/selftest-run-tests.c:66
> 0xb36262 toplev::run_self_tests()
> ../../gcc-trunk/gcc/toplev.c:2074
>
>
> the aarch64 was configured this way:
>
> ../gcc-trunk/configure --prefix=/home/ed/gnu/aarch64-unknown-elf --target=aarch64-unknown-elf --enable-languages=c --disable-shared --disable-threads --disable-libssp --disable-libgomp --disable-libquadmath --disable-libatomic
>
>
> Maybe one last comment on your patch itself, could you please move
> the unit test cases in extra unit test files, or even a self-test tree?
That's one of the things we debated for quite a while earlier. For now
they're living in the source files which they're testing. Whether or
not that's best remains to be seen.
jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 9/9] cse.c selftests
2016-09-09 23:17 ` Jeff Law
@ 2016-09-10 10:17 ` Bernd Edlinger
0 siblings, 0 replies; 7+ messages in thread
From: Bernd Edlinger @ 2016-09-10 10:17 UTC (permalink / raw)
To: Jeff Law, David Malcolm; +Cc: GCC Patches
On 09/10/16 01:11, Jeff Law wrote:
>> First the good news. The unit test does actually work, but not only
>> on aarch even without -mabi=ilp32, but also on i386. All that's
>> missing is a check that we don't get a reg/f here:
> Sweet. This starts to touch on some of those unanswered questions --
> while 71779 was visibly only failing on aarch64, can we build tests
> which are ultimately target independent? I suspected that would be the
> case for 71779, but whether or not that will hold in general remains to
> be seen.
>
Things can be target dependent, like Pmode, ptr_mode as in this case.
And target hooks can detect the cheat...
So I would not be surprised, that a test case like that needs really to
be verified on each arch, if it is actually able to detect the failure,
because if not it just risks a bootstrap failure nothing more.
>>
>> Maybe one last comment on your patch itself, could you please move
>> the unit test cases in extra unit test files, or even a self-test tree?
> That's one of the things we debated for quite a while earlier. For now
> they're living in the source files which they're testing. Whether or
> not that's best remains to be seen.
>
Yes, and for the initial version that is perfectly OK.
I would not have been able to write down the test case, but this looks
like a very powerful method.
The more examples there are the easier it will be to create new ones.
If there are going to be more test cases, it would be better to
have them separated in single files, like in the testsuite.
If there is a standard way how to link the test cases in the
back-end, like with static constructors, adding each test case
to a list, without knowing in advance which ones it will be.
Then we could boot-strap as usual, without risking a
boot-strap failure if a single test case fails.
Then when the test suite runs, it can compile 100 test cases
at a time, link that into a special cc1, and execute that,
then the next 100 test cases, and so on. And the tests
can specify the supported target in a dg-do like target
comment.
Sorry if that has already been discussed somewhere.
Thanks
Bernd.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 0/9] RFC: selftests based on RTL dumps
@ 2016-09-09 0:01 David Malcolm
2016-09-09 0:01 ` [PATCH 9/9] cse.c selftests David Malcolm
0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2016-09-09 0:01 UTC (permalink / raw)
To: gcc-patches; +Cc: 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 | 28 +-
gcc/print-rtl.c | 4 +-
gcc/read-md.c | 328 +++++++++----
gcc/read-md.h | 192 ++++++--
gcc/read-rtl-function.c | 1197 ++++++++++++++++++++++++++++++++++++++++++++++
gcc/read-rtl-function.h | 29 ++
gcc/read-rtl.c | 760 ++++++++++++++++++++++++++++-
gcc/rtl.h | 4 +
gcc/selftest-rtl.c | 81 ++++
gcc/selftest-rtl.h | 66 +++
gcc/selftest-run-tests.c | 11 +
gcc/selftest.c | 60 +++
gcc/selftest.h | 46 ++
gcc/tree-dfa.c | 5 +
33 files changed, 3410 insertions(+), 233 deletions(-)
create mode 100644 gcc/read-rtl-function.c
create mode 100644 gcc/read-rtl-function.h
create mode 100644 gcc/selftest-rtl.c
create mode 100644 gcc/selftest-rtl.h
--
1.8.5.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 9/9] cse.c selftests
2016-09-09 0:01 [PATCH 0/9] RFC: selftests based on RTL dumps David Malcolm
@ 2016-09-09 0:01 ` David Malcolm
2016-09-16 20:34 ` Jeff Law
0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2016-09-09 0:01 UTC (permalink / raw)
To: gcc-patches; +Cc: David Malcolm
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.
---
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. */
+
+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"
+ );
+ rtl_dump_test t (input_dump, 100);
+ dataflow_test df_test;
+
+ int tem;
+ tem = cse_main (get_insns (), max_reg_num ());
+ ASSERT_EQ (0, tem);
+
+ /* Verify that insn 2's SET_SRC has been replaced with
+ the SET_DEST of insn 1. */
+ ASSERT_EQ (SET_DEST (PATTERN (get_insn_by_uid (1))),
+ SET_SRC (PATTERN (get_insn_by_uid (2))));
+}
+
+/* Towards a regression test for PR 71779. */
+
+static void
+test_pr71779 ()
+{
+ /* Only run this tests for target==aarch64. */
+#ifndef GCC_AARCH64_H
+ return;
+#endif
+
+ /* 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"
+ /* Extra insn, to avoid all of the above from being deleted by DCE. */
+ "(insn 1049 1048 0 2 (set (mem:DI (reg:DI 191) [1 i+0 S4 A32])\n"
+ " (const_int 1 [0x1])) -1 (nil))\n");
+
+ rtl_dump_test t (input_dump);
+ dataflow_test df_test;
+
+ int tem;
+ tem = cse_main (get_insns (), max_reg_num ());
+ ASSERT_EQ (0, tem);
+}
+
+/* Run all of the selftests within this file. */
+
+void
+cse_c_tests ()
+{
+ test_simple_cse ();
+ test_pr71779 ();
+}
+
+} // namespace selftest
+#endif /* CHECKING_P */
diff --git a/gcc/selftest-run-tests.c b/gcc/selftest-run-tests.c
index 015572c..5fdfb42 100644
--- a/gcc/selftest-run-tests.c
+++ b/gcc/selftest-run-tests.c
@@ -65,6 +65,7 @@ selftest::run_tests ()
rtl_tests_c_tests ();
read_rtl_function_c_tests ();
df_core_c_tests ();
+ cse_c_tests ();
/* Higher-level tests, or for components that other selftests don't
rely on. */
diff --git a/gcc/selftest.h b/gcc/selftest.h
index 6ad6c88..c0dc862 100644
--- a/gcc/selftest.h
+++ b/gcc/selftest.h
@@ -191,6 +191,7 @@ extern void forcibly_ggc_collect ();
alphabetical order. */
extern void bitmap_c_tests ();
extern void combine_c_tests ();
+extern void cse_c_tests ();
extern void df_core_c_tests ();
extern void diagnostic_c_tests ();
extern void diagnostic_show_locus_c_tests ();
--
1.8.5.3
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 9/9] cse.c selftests
2016-09-09 0:01 ` [PATCH 9/9] cse.c selftests David Malcolm
@ 2016-09-16 20:34 ` Jeff Law
2016-09-16 21:28 ` David Malcolm
0 siblings, 1 reply; 7+ messages in thread
From: Jeff Law @ 2016-09-16 20:34 UTC (permalink / raw)
To: David Malcolm, gcc-patches
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?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 9/9] cse.c selftests
2016-09-16 20:34 ` Jeff Law
@ 2016-09-16 21:28 ` David Malcolm
2016-09-19 17:37 ` Jeff Law
0 siblings, 1 reply; 7+ messages in thread
From: David Malcolm @ 2016-09-16 21:28 UTC (permalink / raw)
To: Jeff Law, gcc-patches
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 9/9] cse.c selftests
2016-09-16 21:28 ` David Malcolm
@ 2016-09-19 17:37 ` Jeff Law
0 siblings, 0 replies; 7+ messages in thread
From: Jeff Law @ 2016-09-19 17:37 UTC (permalink / raw)
To: David Malcolm, gcc-patches
On 09/16/2016 03:19 PM, David Malcolm wrote:
>
>> 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.
It's an alternative, but not one I particularly like since those tests
won't be consistently run. With an abstracted target like Bernd
suggests we ought to be able to make most tests work with the abstracted
target and minimize the number of truely target specific tests.
>> 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.
Wow. I must say I'm surprised. Good news though.
>> +
>>> + /* 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.
What about putting them inside the appropriate gcc.target directories?
We could have one for the "generic" target assuming we build something
like that, the others could live in their target specific directory.
jeff
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-09-19 17:31 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-09 13:32 [PATCH 9/9] cse.c selftests Bernd Edlinger
2016-09-09 23:17 ` Jeff Law
2016-09-10 10:17 ` Bernd Edlinger
-- strict thread matches above, loose matches on Subject: below --
2016-09-09 0:01 [PATCH 0/9] RFC: selftests based on RTL dumps David Malcolm
2016-09-09 0:01 ` [PATCH 9/9] cse.c selftests David Malcolm
2016-09-16 20:34 ` Jeff Law
2016-09-16 21:28 ` David Malcolm
2016-09-19 17:37 ` Jeff Law
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).