* Mostly rewrite genrecog
@ 2015-04-27 10:20 Richard Sandiford
2015-04-28 23:18 ` Jeff Law
` (6 more replies)
0 siblings, 7 replies; 30+ messages in thread
From: Richard Sandiford @ 2015-04-27 10:20 UTC (permalink / raw)
To: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 16385 bytes --]
I think it's been the case for a while that parallel builds of GCC tend
to serialise around the compilation of insn-recog.c, especially with
higher --enable-checking settings. This patch tries to speed that
up by replacing most of genrecog with a new algorithm.
I think the main problems with the current code are:
1. Vector architectures have added lots of new instructions that have
a similar shape and differ only in mode, code or unspec number.
The current algorithm doesn't have any way of factoring out those
similarities.
2. When matching a particular instruction, the current code examines
everything about a SET_DEST before moving on to the SET_SRC. This has
two subproblems:
2a. The destination of a SET isn't very distinctive. It's usually
just a register_operand, a memory_operand, a nonimmediate_operand
or a flags register. We therefore tend to backtrack to the
SET_DEST a lot, oscillating between groups of instructions with
the same kind of destination.
2b. Backtracking through predicate checks is relatively expensive.
It would be good to narrow down the "shape" of the instruction
first and only then check the predicates. (The backtracking is
expensive in terms of insn-recog.o compile time too, both because
we need to copy into argument registers and out of the result
register, and because it adds more sites where spills are needed.)
3. The code keeps one local variable per rtx depth, so it ends up
loading the same rtx many times over (mostly when backtracking).
This is very expensive in rtl-checking builds because each XEXP
includes a code check and a line-specific failure call.
In principle the idea of having one local variable per depth
is good. But it was originally written that way when all optimisations
were done at the rtl level and I imagine each local variable mapped
to one pseudo register. These days the statements that reload the
value needed on backtracking lead to many more SSA names and phi
statements than you'd get with just a single variable per position
(loaded once, so naturally SSA). There is still the potential benefit
of avoiding having sibling rtxes live at once, but fixing (2) above
reduces that problem.
Also, the code is all goto-based, which makes it rather hard to step through.
The patch deals with these as follows:
1. Detect subpatterns that differ only by mode, code and/or integer
(e.g. unspec number) and split them out into a common routine.
2. Match the "shape" of the instruction first, in terms of codes,
integers and vector lengths, and only then check the modes, predicates
and dups. When checking the shape, handle SET_SRCs before SET_DESTs.
In practice this seems to greatly reduce the amount of backtracking.
3. Have one local variable per rtx position. I tested the patch with
and without the change and it helped a lot with rtl-checking builds
without seeming to affect release builds much either way.
As far as debuggability goes, the new code avoids gotos and just
uses "natural" control flow.
The headline stat is that a stage 3 --enable-checking=yes,rtl,df
build of insn-recog.c on my box goes from 7m43s to 2m2s (using the
same stage 2 compiler). The corresponding --enable-checking=release
change is from 49s to 24s (less impressive, as expected).
The patch seems to speed up recog. E.g. the time taken to build
fold-const.ii goes from 6.74s before the patch to 6.69s after it;
not a big speed-up, but reproducible.
Here's a comparison of the number of lines of code in insn-recog.c
before and after the patch on one target per config/ CPU:
aarch64-linux-gnueabi 115526 38169 : 33.04%
alpha-linux-gnu 24479 10740 : 43.87%
arm-linux-gnueabi 169208 67759 : 40.04%
avr-rtems 55647 22127 : 39.76%
bfin-elf 13928 6498 : 46.65%
c6x-elf 29928 13324 : 44.52%
cr16-elf 2650 1419 : 53.55%
cris-elf 18669 7257 : 38.87%
epiphany-elf 19308 6131 : 31.75%
fr30-elf 2204 1112 : 50.45%
frv-linux-gnu 13541 5950 : 43.94%
h8300-elf 19584 9327 : 47.63%
hppa64-hp-hpux11.23 18299 8549 : 46.72%
ia64-linux-gnu 37629 17101 : 45.45%
iq2000-elf 2752 1609 : 58.47%
lm32-elf 1536 872 : 56.77%
m32c-elf 10040 4145 : 41.28%
m32r-elf 4436 2307 : 52.01%
m68k-linux-gnu 15739 8147 : 51.76%
mcore-elf 4816 2577 : 53.51%
mep-elf 67335 15929 : 23.66%
microblaze-elf 2656 1587 : 59.75%
mips-linux-gnu 54543 24186 : 44.34%
mmix 2597 1487 : 57.26%
mn10300-elf 6384 3294 : 51.60%
moxie-elf 1311 659 : 50.27%
msp430-elf 6054 2382 : 39.35%
nds32le-elf 5953 3152 : 52.95%
nios2-linux-gnu 3735 2143 : 57.38%
pdp11 2137 1157 : 54.14%
powerpc-eabispe 109322 40582 : 37.12%
powerpc-linux-gnu 82976 32192 : 38.80%
rl78-elf 5321 2432 : 45.71%
rx-elf 14454 7534 : 52.12%
s390-linux-gnu 48487 20454 : 42.18%
sh-linux-gnu 104087 41820 : 40.18%
sparc-linux-gnu 21912 10509 : 47.96%
spu-elf 19937 8182 : 41.04%
tilegx-elf 15412 6970 : 45.22%
tilepro-elf 11998 5479 : 45.67%
v850-elf 8725 4438 : 50.87%
vax-netbsdelf 4537 2410 : 53.12%
visium-elf 15190 7224 : 47.56%
x86_64-darwin 346396 119593 : 34.52%
xstormy16-elf 4660 2229 : 47.83%
xtensa-elf 2799 1514 : 54.09%
Here's the loadable size of insn-recog.o in an --enable-checking=release
build on an x86_64-linux-gnu box:
aarch64-linux-gnueabi 443955 298026 : 67.13%
alpha-linux-gnu 97194 80893 : 83.23%
arm-linux-gnueabi 782325 632248 : 80.82%
avr-rtems 226827 159763 : 70.43%
bfin-elf 52563 42376 : 80.62%
c6x-elf 112512 90142 : 80.12%
cr16-elf 10446 10006 : 95.79%
cris-elf 74771 52634 : 70.39%
epiphany-elf 87577 52284 : 59.70%
fr30-elf 8041 7713 : 95.92%
frv-linux-gnu 53699 47543 : 88.54%
h8300-elf 70708 66274 : 93.73%
hppa64-hp-hpux11.23 71597 57484 : 80.29%
ia64-linux-gnu 147286 130632 : 88.69%
iq2000-elf 11002 11774 : 107.02%
lm32-elf 5894 5798 : 98.37%
m32c-elf 36563 28855 : 78.92%
m32r-elf 17252 16910 : 98.02%
m68k-linux-gnu 58248 59781 : 102.63%
mcore-elf 17708 18948 : 107.00%
mep-elf 314466 150771 : 47.95%
microblaze-elf 10257 10534 : 102.70%
mips-linux-gnu 230991 191155 : 82.75%
mmix 10782 10678 : 99.04%
mn10300-elf 24035 22802 : 94.87%
moxie-elf 4622 4198 : 90.83%
msp430-elf 21707 16539 : 76.19%
nds32le-elf 22041 19444 : 88.22%
nios2-linux-gnu 15595 13238 : 84.89%
pdp11 7630 8254 : 108.18%
powerpc-eabispe 430816 308481 : 71.60%
powerpc-linux-gnu 317738 248534 : 78.22%
rl78-elf 18904 16329 : 86.38%
rx-elf 55015 56632 : 102.94%
s390-linux-gnu 190584 148961 : 78.16%
sh-linux-gnu 408446 307927 : 75.39%
sparc-linux-gnu 91016 80640 : 88.60%
spu-elf 80387 69151 : 86.02%
tilegx-elf 63815 49977 : 78.32%
tilepro-elf 51763 39252 : 75.83%
v850-elf 32812 28462 : 86.74%
vax-netbsdelf 18350 18259 : 99.50%
visium-elf 56872 46790 : 82.27%
x86_64-darwin 1306182 883169 : 67.61%
xstormy16-elf 17044 14430 : 84.66%
xtensa-elf 10780 9678 : 89.78%
The same for --enable-checking=yes,rtl:
aarch64-linux-gnueabi 1790272 507488 : 28.35%
alpha-linux-gnu 440058 193826 : 44.05%
arm-linux-gnueabi 2845568 1299337 : 45.66%
avr-rtems 885672 294387 : 33.24%
bfin-elf 280606 142836 : 50.90%
c6x-elf 486345 259256 : 53.31%
cr16-elf 46626 20044 : 42.99%
cris-elf 426813 144414 : 33.84%
epiphany-elf 353078 122166 : 34.60%
fr30-elf 40414 21042 : 52.07%
frv-linux-gnu 259550 111335 : 42.90%
h8300-elf 355199 158411 : 44.60%
hppa64-hp-hpux11.23 340584 149009 : 43.75%
ia64-linux-gnu 661364 293710 : 44.41%
iq2000-elf 41123 26709 : 64.95%
lm32-elf 20370 14781 : 72.56%
m32c-elf 174344 62000 : 35.56%
m32r-elf 74357 41773 : 56.18%
m68k-linux-gnu 275733 117445 : 42.59%
mcore-elf 85180 48018 : 56.37%
mep-elf 1450168 376020 : 25.93%
microblaze-elf 44189 26295 : 59.51%
mips-linux-gnu 876650 375753 : 42.86%
mmix 49882 25363 : 50.85%
mn10300-elf 128148 66768 : 52.10%
moxie-elf 23388 9011 : 38.53%
msp430-elf 114200 34426 : 30.15%
nds32le-elf 101416 73677 : 72.65%
nios2-linux-gnu 58799 29825 : 50.72%
pdp11 32836 18557 : 56.51%
powerpc-eabispe 1976098 626942 : 31.73%
powerpc-linux-gnu 1510652 526841 : 34.88%
rl78-elf 93675 40538 : 43.28%
rx-elf 279748 137284 : 49.07%
s390-linux-gnu 857009 316494 : 36.93%
sh-linux-gnu 2154337 806571 : 37.44%
sparc-linux-gnu 367682 169019 : 45.97%
spu-elf 341945 135629 : 39.66%
tilegx-elf 235480 112103 : 47.61%
tilepro-elf 246231 104137 : 42.29%
v850-elf 158028 72875 : 46.12%
vax-netbsdelf 85057 37578 : 44.18%
visium-elf 257148 103331 : 40.18%
x86_64-darwin 5514235 1721777 : 31.22%
xstormy16-elf 83456 46128 : 55.27%
xtensa-elf 52652 29880 : 56.75%
Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-none-eabi.
Also tested by building the testsuite for each of the targets above
and making sure there were no assembly differences. Made sure that no
objects in spec2k6 changed for aarch64-linux-gnu (except for perlbench
perl.o and cactusADM datestamp.o, where the differences are timestamps).
OK to install?
Thanks,
Richard
PS. I've attached the new genrecog.c since the diff version is unreadable.
gcc/
* Makefile.in (build/genrecog.o): Depend on inchash.h.
(build/genrecog$(build_exeext): Depend on build/hash-table.o and
build/inchash.o
* genrecog.c: Rewrite most of the code except for the third page.
Index: gcc/Makefile.in
===================================================================
--- gcc/Makefile.in 2015-04-27 10:42:57.783191573 +0100
+++ gcc/Makefile.in 2015-04-27 10:43:42.878643078 +0100
@@ -2527,7 +2527,8 @@ build/genpeep.o : genpeep.c $(RTL_BASE_H
build/genpreds.o : genpreds.c $(RTL_BASE_H) $(BCONFIG_H) $(SYSTEM_H) \
coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h $(OBSTACK_H)
build/genrecog.o : genrecog.c $(RTL_BASE_H) $(BCONFIG_H) $(SYSTEM_H) \
- coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h
+ coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h \
+ $(HASH_TABLE_H) inchash.h
build/genhooks.o : genhooks.c $(TARGET_DEF) $(C_TARGET_DEF) \
$(COMMON_TARGET_DEF) $(BCONFIG_H) $(SYSTEM_H) errors.h
build/genmddump.o : genmddump.c $(RTL_BASE_H) $(BCONFIG_H) $(SYSTEM_H) \
@@ -2559,6 +2560,8 @@ genprog = $(genprogerr) check checksum c
# These programs need libs over and above what they get from the above list.
build/genautomata$(build_exeext) : BUILD_LIBS += -lm
+build/genrecog$(build_exeext) : build/hash-table.o build/inchash.o
+
# For stage1 and when cross-compiling use the build libcpp which is
# built with NLS disabled. For stage2+ use the host library and
# its dependencies.
[-- Attachment #2: genrecog.c.bz2 --]
[-- Type: application/octet-stream, Size: 34987 bytes --]
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: Mostly rewrite genrecog 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford @ 2015-04-28 23:18 ` Jeff Law 2015-04-29 13:59 ` Richard Sandiford 2015-04-29 8:52 ` Eric Botcazou ` (5 subsequent siblings) 6 siblings, 1 reply; 30+ messages in thread From: Jeff Law @ 2015-04-28 23:18 UTC (permalink / raw) To: gcc-patches, richard.sandiford On 04/27/2015 04:20 AM, Richard Sandiford wrote: > I think it's been the case for a while that parallel builds of GCC tend > to serialise around the compilation of insn-recog.c, especially with > higher --enable-checking settings. This patch tries to speed that > up by replacing most of genrecog with a new algorithm. > > I think the main problems with the current code are: > > 1. Vector architectures have added lots of new instructions that have > a similar shape and differ only in mode, code or unspec number. > The current algorithm doesn't have any way of factoring out those > similarities. > > 2. When matching a particular instruction, the current code examines > everything about a SET_DEST before moving on to the SET_SRC. This has > two subproblems: > > 2a. The destination of a SET isn't very distinctive. It's usually > just a register_operand, a memory_operand, a nonimmediate_operand > or a flags register. We therefore tend to backtrack to the > SET_DEST a lot, oscillating between groups of instructions with > the same kind of destination. > > 2b. Backtracking through predicate checks is relatively expensive. > It would be good to narrow down the "shape" of the instruction > first and only then check the predicates. (The backtracking is > expensive in terms of insn-recog.o compile time too, both because > we need to copy into argument registers and out of the result > register, and because it adds more sites where spills are needed.) > > 3. The code keeps one local variable per rtx depth, so it ends up > loading the same rtx many times over (mostly when backtracking). > This is very expensive in rtl-checking builds because each XEXP > includes a code check and a line-specific failure call. > > In principle the idea of having one local variable per depth > is good. But it was originally written that way when all optimisations > were done at the rtl level and I imagine each local variable mapped > to one pseudo register. These days the statements that reload the > value needed on backtracking lead to many more SSA names and phi > statements than you'd get with just a single variable per position > (loaded once, so naturally SSA). There is still the potential benefit > of avoiding having sibling rtxes live at once, but fixing (2) above > reduces that problem. > > Also, the code is all goto-based, which makes it rather hard to step through. > > The patch deals with these as follows: > > 1. Detect subpatterns that differ only by mode, code and/or integer > (e.g. unspec number) and split them out into a common routine. > > 2. Match the "shape" of the instruction first, in terms of codes, > integers and vector lengths, and only then check the modes, predicates > and dups. When checking the shape, handle SET_SRCs before SET_DESTs. > In practice this seems to greatly reduce the amount of backtracking. > > 3. Have one local variable per rtx position. I tested the patch with > and without the change and it helped a lot with rtl-checking builds > without seeming to affect release builds much either way. > > As far as debuggability goes, the new code avoids gotos and just > uses "natural" control flow. > > The headline stat is that a stage 3 --enable-checking=yes,rtl,df > build of insn-recog.c on my box goes from 7m43s to 2m2s (using the > same stage 2 compiler). The corresponding --enable-checking=release > change is from 49s to 24s (less impressive, as expected). > > The patch seems to speed up recog. E.g. the time taken to build > fold-const.ii goes from 6.74s before the patch to 6.69s after it; > not a big speed-up, but reproducible. [ big snip ] I don't see anything that makes me think we don't want this :-) It's interesting to note that the regressions in loadable size of a release-checking insn-recog aren't any mainstream ports. Hell, I'd probably claim they're all fringe ports (iq2000, m68k, mcore, microblaze, pdp11, rx) > Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-none-eabi. > Also tested by building the testsuite for each of the targets above > and making sure there were no assembly differences. Made sure that no > objects in spec2k6 changed for aarch64-linux-gnu (except for perlbench > perl.o and cactusADM datestamp.o, where the differences are timestamps). > OK to install? To be very blunt, I'm probably not capable of reviewing the new code. You're going to know it best and you should probably own it. Given your history with gcc and attention to detail, I'm comfortable with approving this knowing you'll deal with any issues as they arise. The one thing I would ask is that you make sure to include the recently added matching constraint checking bits in genrecog. I'm assuming all the older sanity checks that have been in genrecog for a longer period of time have been kept. Jeff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-28 23:18 ` Jeff Law @ 2015-04-29 13:59 ` Richard Sandiford 0 siblings, 0 replies; 30+ messages in thread From: Richard Sandiford @ 2015-04-29 13:59 UTC (permalink / raw) To: Jeff Law; +Cc: gcc-patches Jeff Law <law@redhat.com> writes: > On 04/27/2015 04:20 AM, Richard Sandiford wrote: >> Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-none-eabi. >> Also tested by building the testsuite for each of the targets above >> and making sure there were no assembly differences. Made sure that no >> objects in spec2k6 changed for aarch64-linux-gnu (except for perlbench >> perl.o and cactusADM datestamp.o, where the differences are timestamps). >> OK to install? > To be very blunt, I'm probably not capable of reviewing the new code. > You're going to know it best and you should probably own it. > > Given your history with gcc and attention to detail, I'm comfortable > with approving this knowing you'll deal with any issues as they arise. Thanks, applied. > The one thing I would ask is that you make sure to include the recently > added matching constraint checking bits in genrecog. I'm assuming all > the older sanity checks that have been in genrecog for a longer period > of time have been kept. Yeah, Chen Gang's changes are all still in there. All the older checks should still be in there too. Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford 2015-04-28 23:18 ` Jeff Law @ 2015-04-29 8:52 ` Eric Botcazou 2015-04-29 13:51 ` Richard Sandiford 2015-04-30 6:54 ` Bin.Cheng ` (4 subsequent siblings) 6 siblings, 1 reply; 30+ messages in thread From: Eric Botcazou @ 2015-04-29 8:52 UTC (permalink / raw) To: Richard Sandiford; +Cc: gcc-patches > I think it's been the case for a while that parallel builds of GCC tend > to serialise around the compilation of insn-recog.c, especially with > higher --enable-checking settings. This patch tries to speed that > up by replacing most of genrecog with a new algorithm. I can confirm this, especially with --enable-checking=rtl. > Also, the code is all goto-based, which makes it rather hard to step > through. Do you mean the code in genrecog.c or the generated code in insn-recog.c? > The patch deals with these as follows: > > 1. Detect subpatterns that differ only by mode, code and/or integer > (e.g. unspec number) and split them out into a common routine. > > 2. Match the "shape" of the instruction first, in terms of codes, > integers and vector lengths, and only then check the modes, predicates > and dups. When checking the shape, handle SET_SRCs before SET_DESTs. > In practice this seems to greatly reduce the amount of backtracking. > > 3. Have one local variable per rtx position. I tested the patch with > and without the change and it helped a lot with rtl-checking builds > without seeming to affect release builds much either way. > > As far as debuggability goes, the new code avoids gotos and just > uses "natural" control flow. See below. > The headline stat is that a stage 3 --enable-checking=yes,rtl,df > build of insn-recog.c on my box goes from 7m43s to 2m2s (using the > same stage 2 compiler). The corresponding --enable-checking=release > change is from 49s to 24s (less impressive, as expected). The first figure is quite impressive, great work! > PS. I've attached the new genrecog.c since the diff version is unreadable. Small request: you didn't change a single line of the head comment, yet the size of the file is doubled. Could you add a sketch of the algorithm to the head comment, e.g. by copy-and-pasting the above part of your message? The old code contained a couple of DEBUG_FUNCTIONs but the new one doesn't. -- Eric Botcazou ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-29 8:52 ` Eric Botcazou @ 2015-04-29 13:51 ` Richard Sandiford 2015-04-30 10:44 ` Eric Botcazou 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-04-29 13:51 UTC (permalink / raw) To: Eric Botcazou; +Cc: gcc-patches Eric Botcazou <ebotcazou@adacore.com> writes: >> Also, the code is all goto-based, which makes it rather hard to step >> through. > > Do you mean the code in genrecog.c or the generated code in insn-recog.c? The generated code. genrecog.c itself isn't bad. :-) >> PS. I've attached the new genrecog.c since the diff version is unreadable. > > Small request: you didn't change a single line of the head comment, yet the > size of the file is doubled. Could you add a sketch of the algorithm to the > head comment, e.g. by copy-and-pasting the above part of your message? OK. I'd left the head comment alone because it just described the interface, which hasn't changed. But I suppose past lack of commentary doesn't justify future lack of commentary. Here's what I added: At a high level, the algorithm used in this file is as follows: 1. Build up a decision tree for each routine, using the following approach to matching an rtx: - First determine the "shape" of the rtx, based on GET_CODE, XVECLEN and XINT. This phase examines SET_SRCs before SET_DESTs since SET_SRCs tend to be more distinctive. It examines other operands in numerical order, since the canonicalization rules prefer putting complex operands of commutative operators first. - Next check modes and predicates. This phase examines all operands in numerical order, even for SETs, since the mode of a SET_DEST is exact while the mode of a SET_SRC can be VOIDmode for constant integers. - Next check match_dups. - Finally check the C condition and (where appropriate) pnum_clobbers. 2. Try to optimize the tree by removing redundant tests, CSEing tests, folding tests together, etc. 3. Look for common subtrees and split them out into "pattern" routines. These common subtrees can be identical or they can differ in mode, code, or integer (usually an UNSPEC or UNSPEC_VOLATILE code). In the latter case the users of the pattern routine pass the appropriate mode, etc., as argument. For example, if two patterns contain: (plus:SI (match_operand:SI 1 "register_operand") (match_operand:SI 2 "register_operand")) we can split the associated matching code out into a subroutine. If a pattern contains: (minus:DI (match_operand:DI 1 "register_operand") (match_operand:DI 2 "register_operand")) then we can consider using the same matching routine for both the plus and minus expressions, passing PLUS and SImode in the former case and MINUS and DImode in the latter case. The main aim of this phase is to reduce the compile time of the insn-recog.c code and to reduce the amount of object code in insn-recog.o. 4. Split the matching trees into functions, trying to limit the size of each function to a sensible amount. Again, the main aim of this phase is to reduce the compile time of insn-recog.c. (It doesn't help with the size of insn-recog.o.) 5. Write out C++ code for each function. BTW, hope at least part of the doubling in size is due to more commentary in the code itself. > The old code contained a couple of DEBUG_FUNCTIONs but the new one doesn't. Yeah, but I don't know how useful they were. I had to debug the old code several times and never used them. I'd rather leave stuff like that to someone who wants it rather than try to write routines speculatively in the hope that someone would find them useful. Thanks, Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-29 13:51 ` Richard Sandiford @ 2015-04-30 10:44 ` Eric Botcazou 0 siblings, 0 replies; 30+ messages in thread From: Eric Botcazou @ 2015-04-30 10:44 UTC (permalink / raw) To: Richard Sandiford; +Cc: gcc-patches > The generated code. genrecog.c itself isn't bad. :-) Nice work then. > OK. I'd left the head comment alone because it just described the > interface, which hasn't changed. But I suppose past lack of commentary > doesn't justify future lack of commentary. Here's what I added: > [...] > BTW, hope at least part of the doubling in size is due to more commentary > in the code itself. I see. Thanks a lot for writing down the description of the algorithm! > I'd rather leave stuff like that to someone who wants it rather than try > to write routines speculatively in the hope that someone would find them > useful. OK. -- Eric Botcazou ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford 2015-04-28 23:18 ` Jeff Law 2015-04-29 8:52 ` Eric Botcazou @ 2015-04-30 6:54 ` Bin.Cheng 2015-04-30 11:58 ` Richard Sandiford 2015-04-30 7:17 ` Andreas Schwab ` (3 subsequent siblings) 6 siblings, 1 reply; 30+ messages in thread From: Bin.Cheng @ 2015-04-30 6:54 UTC (permalink / raw) To: gcc-patches List, richard.sandiford On Mon, Apr 27, 2015 at 6:20 PM, Richard Sandiford <richard.sandiford@arm.com> wrote: > I think it's been the case for a while that parallel builds of GCC tend > to serialise around the compilation of insn-recog.c, especially with > higher --enable-checking settings. This patch tries to speed that > up by replacing most of genrecog with a new algorithm. > > I think the main problems with the current code are: > > 1. Vector architectures have added lots of new instructions that have > a similar shape and differ only in mode, code or unspec number. > The current algorithm doesn't have any way of factoring out those > similarities. > > 2. When matching a particular instruction, the current code examines > everything about a SET_DEST before moving on to the SET_SRC. This has > two subproblems: > > 2a. The destination of a SET isn't very distinctive. It's usually > just a register_operand, a memory_operand, a nonimmediate_operand > or a flags register. We therefore tend to backtrack to the > SET_DEST a lot, oscillating between groups of instructions with > the same kind of destination. > > 2b. Backtracking through predicate checks is relatively expensive. > It would be good to narrow down the "shape" of the instruction > first and only then check the predicates. (The backtracking is > expensive in terms of insn-recog.o compile time too, both because > we need to copy into argument registers and out of the result > register, and because it adds more sites where spills are needed.) > > 3. The code keeps one local variable per rtx depth, so it ends up > loading the same rtx many times over (mostly when backtracking). > This is very expensive in rtl-checking builds because each XEXP > includes a code check and a line-specific failure call. > > In principle the idea of having one local variable per depth > is good. But it was originally written that way when all optimisations > were done at the rtl level and I imagine each local variable mapped > to one pseudo register. These days the statements that reload the > value needed on backtracking lead to many more SSA names and phi > statements than you'd get with just a single variable per position > (loaded once, so naturally SSA). There is still the potential benefit > of avoiding having sibling rtxes live at once, but fixing (2) above > reduces that problem. > > Also, the code is all goto-based, which makes it rather hard to step through. > > The patch deals with these as follows: > > 1. Detect subpatterns that differ only by mode, code and/or integer > (e.g. unspec number) and split them out into a common routine. > > 2. Match the "shape" of the instruction first, in terms of codes, > integers and vector lengths, and only then check the modes, predicates > and dups. When checking the shape, handle SET_SRCs before SET_DESTs. > In practice this seems to greatly reduce the amount of backtracking. > > 3. Have one local variable per rtx position. I tested the patch with > and without the change and it helped a lot with rtl-checking builds > without seeming to affect release builds much either way. > > As far as debuggability goes, the new code avoids gotos and just > uses "natural" control flow. > > The headline stat is that a stage 3 --enable-checking=yes,rtl,df > build of insn-recog.c on my box goes from 7m43s to 2m2s (using the > same stage 2 compiler). The corresponding --enable-checking=release > change is from 49s to 24s (less impressive, as expected). > > The patch seems to speed up recog. E.g. the time taken to build > fold-const.ii goes from 6.74s before the patch to 6.69s after it; > not a big speed-up, but reproducible. > > Here's a comparison of the number of lines of code in insn-recog.c > before and after the patch on one target per config/ CPU: > > aarch64-linux-gnueabi 115526 38169 : 33.04% > alpha-linux-gnu 24479 10740 : 43.87% > arm-linux-gnueabi 169208 67759 : 40.04% > avr-rtems 55647 22127 : 39.76% > bfin-elf 13928 6498 : 46.65% > c6x-elf 29928 13324 : 44.52% > cr16-elf 2650 1419 : 53.55% > cris-elf 18669 7257 : 38.87% > epiphany-elf 19308 6131 : 31.75% > fr30-elf 2204 1112 : 50.45% > frv-linux-gnu 13541 5950 : 43.94% > h8300-elf 19584 9327 : 47.63% > hppa64-hp-hpux11.23 18299 8549 : 46.72% > ia64-linux-gnu 37629 17101 : 45.45% > iq2000-elf 2752 1609 : 58.47% > lm32-elf 1536 872 : 56.77% > m32c-elf 10040 4145 : 41.28% > m32r-elf 4436 2307 : 52.01% > m68k-linux-gnu 15739 8147 : 51.76% > mcore-elf 4816 2577 : 53.51% > mep-elf 67335 15929 : 23.66% > microblaze-elf 2656 1587 : 59.75% > mips-linux-gnu 54543 24186 : 44.34% > mmix 2597 1487 : 57.26% > mn10300-elf 6384 3294 : 51.60% > moxie-elf 1311 659 : 50.27% > msp430-elf 6054 2382 : 39.35% > nds32le-elf 5953 3152 : 52.95% > nios2-linux-gnu 3735 2143 : 57.38% > pdp11 2137 1157 : 54.14% > powerpc-eabispe 109322 40582 : 37.12% > powerpc-linux-gnu 82976 32192 : 38.80% > rl78-elf 5321 2432 : 45.71% > rx-elf 14454 7534 : 52.12% > s390-linux-gnu 48487 20454 : 42.18% > sh-linux-gnu 104087 41820 : 40.18% > sparc-linux-gnu 21912 10509 : 47.96% > spu-elf 19937 8182 : 41.04% > tilegx-elf 15412 6970 : 45.22% > tilepro-elf 11998 5479 : 45.67% > v850-elf 8725 4438 : 50.87% > vax-netbsdelf 4537 2410 : 53.12% > visium-elf 15190 7224 : 47.56% > x86_64-darwin 346396 119593 : 34.52% > xstormy16-elf 4660 2229 : 47.83% > xtensa-elf 2799 1514 : 54.09% > > Here's the loadable size of insn-recog.o in an --enable-checking=release > build on an x86_64-linux-gnu box: > > aarch64-linux-gnueabi 443955 298026 : 67.13% > alpha-linux-gnu 97194 80893 : 83.23% > arm-linux-gnueabi 782325 632248 : 80.82% > avr-rtems 226827 159763 : 70.43% > bfin-elf 52563 42376 : 80.62% > c6x-elf 112512 90142 : 80.12% > cr16-elf 10446 10006 : 95.79% > cris-elf 74771 52634 : 70.39% > epiphany-elf 87577 52284 : 59.70% > fr30-elf 8041 7713 : 95.92% > frv-linux-gnu 53699 47543 : 88.54% > h8300-elf 70708 66274 : 93.73% > hppa64-hp-hpux11.23 71597 57484 : 80.29% > ia64-linux-gnu 147286 130632 : 88.69% > iq2000-elf 11002 11774 : 107.02% > lm32-elf 5894 5798 : 98.37% > m32c-elf 36563 28855 : 78.92% > m32r-elf 17252 16910 : 98.02% > m68k-linux-gnu 58248 59781 : 102.63% > mcore-elf 17708 18948 : 107.00% > mep-elf 314466 150771 : 47.95% > microblaze-elf 10257 10534 : 102.70% > mips-linux-gnu 230991 191155 : 82.75% > mmix 10782 10678 : 99.04% > mn10300-elf 24035 22802 : 94.87% > moxie-elf 4622 4198 : 90.83% > msp430-elf 21707 16539 : 76.19% > nds32le-elf 22041 19444 : 88.22% > nios2-linux-gnu 15595 13238 : 84.89% > pdp11 7630 8254 : 108.18% > powerpc-eabispe 430816 308481 : 71.60% > powerpc-linux-gnu 317738 248534 : 78.22% > rl78-elf 18904 16329 : 86.38% > rx-elf 55015 56632 : 102.94% > s390-linux-gnu 190584 148961 : 78.16% > sh-linux-gnu 408446 307927 : 75.39% > sparc-linux-gnu 91016 80640 : 88.60% > spu-elf 80387 69151 : 86.02% > tilegx-elf 63815 49977 : 78.32% > tilepro-elf 51763 39252 : 75.83% > v850-elf 32812 28462 : 86.74% > vax-netbsdelf 18350 18259 : 99.50% > visium-elf 56872 46790 : 82.27% > x86_64-darwin 1306182 883169 : 67.61% > xstormy16-elf 17044 14430 : 84.66% > xtensa-elf 10780 9678 : 89.78% > > The same for --enable-checking=yes,rtl: > > aarch64-linux-gnueabi 1790272 507488 : 28.35% > alpha-linux-gnu 440058 193826 : 44.05% > arm-linux-gnueabi 2845568 1299337 : 45.66% > avr-rtems 885672 294387 : 33.24% > bfin-elf 280606 142836 : 50.90% > c6x-elf 486345 259256 : 53.31% > cr16-elf 46626 20044 : 42.99% > cris-elf 426813 144414 : 33.84% > epiphany-elf 353078 122166 : 34.60% > fr30-elf 40414 21042 : 52.07% > frv-linux-gnu 259550 111335 : 42.90% > h8300-elf 355199 158411 : 44.60% > hppa64-hp-hpux11.23 340584 149009 : 43.75% > ia64-linux-gnu 661364 293710 : 44.41% > iq2000-elf 41123 26709 : 64.95% > lm32-elf 20370 14781 : 72.56% > m32c-elf 174344 62000 : 35.56% > m32r-elf 74357 41773 : 56.18% > m68k-linux-gnu 275733 117445 : 42.59% > mcore-elf 85180 48018 : 56.37% > mep-elf 1450168 376020 : 25.93% > microblaze-elf 44189 26295 : 59.51% > mips-linux-gnu 876650 375753 : 42.86% > mmix 49882 25363 : 50.85% > mn10300-elf 128148 66768 : 52.10% > moxie-elf 23388 9011 : 38.53% > msp430-elf 114200 34426 : 30.15% > nds32le-elf 101416 73677 : 72.65% > nios2-linux-gnu 58799 29825 : 50.72% > pdp11 32836 18557 : 56.51% > powerpc-eabispe 1976098 626942 : 31.73% > powerpc-linux-gnu 1510652 526841 : 34.88% > rl78-elf 93675 40538 : 43.28% > rx-elf 279748 137284 : 49.07% > s390-linux-gnu 857009 316494 : 36.93% > sh-linux-gnu 2154337 806571 : 37.44% > sparc-linux-gnu 367682 169019 : 45.97% > spu-elf 341945 135629 : 39.66% > tilegx-elf 235480 112103 : 47.61% > tilepro-elf 246231 104137 : 42.29% > v850-elf 158028 72875 : 46.12% > vax-netbsdelf 85057 37578 : 44.18% > visium-elf 257148 103331 : 40.18% > x86_64-darwin 5514235 1721777 : 31.22% > xstormy16-elf 83456 46128 : 55.27% > xtensa-elf 52652 29880 : 56.75% > > Tested on x86_64-linux-gnu, aarch64-linux-gnu and arm-none-eabi. > Also tested by building the testsuite for each of the targets above > and making sure there were no assembly differences. Made sure that no > objects in spec2k6 changed for aarch64-linux-gnu (except for perlbench > perl.o and cactusADM datestamp.o, where the differences are timestamps). > OK to install? > > Thanks, > Richard > > PS. I've attached the new genrecog.c since the diff version is unreadable. > > > gcc/ > * Makefile.in (build/genrecog.o): Depend on inchash.h. > (build/genrecog$(build_exeext): Depend on build/hash-table.o and > build/inchash.o > * genrecog.c: Rewrite most of the code except for the third page. > > Index: gcc/Makefile.in > =================================================================== > --- gcc/Makefile.in 2015-04-27 10:42:57.783191573 +0100 > +++ gcc/Makefile.in 2015-04-27 10:43:42.878643078 +0100 > @@ -2527,7 +2527,8 @@ build/genpeep.o : genpeep.c $(RTL_BASE_H > build/genpreds.o : genpreds.c $(RTL_BASE_H) $(BCONFIG_H) $(SYSTEM_H) \ > coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h $(OBSTACK_H) > build/genrecog.o : genrecog.c $(RTL_BASE_H) $(BCONFIG_H) $(SYSTEM_H) \ > - coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h > + coretypes.h $(GTM_H) errors.h $(READ_MD_H) gensupport.h \ > + $(HASH_TABLE_H) inchash.h > build/genhooks.o : genhooks.c $(TARGET_DEF) $(C_TARGET_DEF) \ > $(COMMON_TARGET_DEF) $(BCONFIG_H) $(SYSTEM_H) errors.h > build/genmddump.o : genmddump.c $(RTL_BASE_H) $(BCONFIG_H) $(SYSTEM_H) \ > @@ -2559,6 +2560,8 @@ genprog = $(genprogerr) check checksum c > # These programs need libs over and above what they get from the above list. > build/genautomata$(build_exeext) : BUILD_LIBS += -lm > > +build/genrecog$(build_exeext) : build/hash-table.o build/inchash.o > + > # For stage1 and when cross-compiling use the build libcpp which is > # built with NLS disabled. For stage2+ use the host library and > # its dependencies. > Hi Richard, I noticed that this patch caused ICE for gcc.target/arm/mmx-2.c on arm-none-linux-gnueabi. Could you please have a look at it? The log message is as below, /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c: In function 'foo': /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c:166:1: error: unrecognizable insn: (insn 541 540 542 2 (set (reg:V4HI 512 [ D.4809 ]) (vec_merge:V4HI (vec_select:V4HI (reg:V4HI 510 [ D.4806 ]) (parallel [ (const_int 2 [0x2]) (const_int 0 [0]) (const_int 3 [0x3]) (const_int 1 [0x1]) ])) (vec_select:V4HI (reg:V4HI 511 [ D.4806 ]) (parallel [ (const_int 0 [0]) (const_int 2 [0x2]) (const_int 1 [0x1]) (const_int 3 [0x3]) ])) (const_int 5 [0x5]))) /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c:159 -1 (nil)) /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c:166:1: internal compiler error: in extract_insn, at recog.c:2341 0xa42d2a _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) /projects/.../src/gcc/gcc/rtl-error.c:110 0xa42d59 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*) /projects/.../src/gcc/gcc/rtl-error.c:118 0xa15ff7 extract_insn(rtx_insn*) /projects/.../src/gcc/gcc/recog.c:2341 0x7ffb42 instantiate_virtual_regs_in_insn /projects/.../src/gcc/gcc/function.c:1598 0x7ffb42 instantiate_virtual_regs /projects/.../src/gcc/gcc/function.c:1966 0x7ffb42 execute /projects/.../src/gcc/gcc/function.c:2015 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <http://gcc.gnu.org/bugs.html> for instructions. GCC is configured with gcc/configure --target=arm-none-linux-gnueabi --prefix= --with-sysroot=... --enable-shared --disable-libsanitizer --disable-libssp --disable-libmudflap --with-plugin-ld=arm-none-linux-gnueabi-ld --enable-checking=yes --enable-languages=c,c++,fortran --with-gmp=... --with-mpfr=... --with-mpc=... --with-isl=... --with-cloog=... --with-arch=armv7-a --with-fpu=vfpv3-d16 --with-float=softfp --with-arch=armv7-a Thanks, bin ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-30 6:54 ` Bin.Cheng @ 2015-04-30 11:58 ` Richard Sandiford 0 siblings, 0 replies; 30+ messages in thread From: Richard Sandiford @ 2015-04-30 11:58 UTC (permalink / raw) To: Bin.Cheng; +Cc: gcc-patches List Bin.Cheng <amker.cheng@gmail.com> writes: > Hi Richard, > I noticed that this patch caused ICE for gcc.target/arm/mmx-2.c on > arm-none-linux-gnueabi. Could you please have a look at it? > > The log message is as below, > /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c: In function 'foo': > /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c:166:1: > error: unrecognizable insn: > (insn 541 540 542 2 (set (reg:V4HI 512 [ D.4809 ]) > (vec_merge:V4HI (vec_select:V4HI (reg:V4HI 510 [ D.4806 ]) > (parallel [ > (const_int 2 [0x2]) > (const_int 0 [0]) > (const_int 3 [0x3]) > (const_int 1 [0x1]) > ])) > (vec_select:V4HI (reg:V4HI 511 [ D.4806 ]) > (parallel [ > (const_int 0 [0]) > (const_int 2 [0x2]) > (const_int 1 [0x1]) > (const_int 3 [0x3]) > ])) > (const_int 5 [0x5]))) > /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c:159 -1 > (nil)) > /projects/.../src/gcc/gcc/testsuite/gcc.target/arm/mmx-2.c:166:1: > internal compiler error: in extract_insn, at recog.c:2341 > 0xa42d2a _fatal_insn(char const*, rtx_def const*, char const*, int, char const*) > /projects/.../src/gcc/gcc/rtl-error.c:110 > 0xa42d59 _fatal_insn_not_found(rtx_def const*, char const*, int, char const*) > /projects/.../src/gcc/gcc/rtl-error.c:118 > 0xa15ff7 extract_insn(rtx_insn*) > /projects/.../src/gcc/gcc/recog.c:2341 > 0x7ffb42 instantiate_virtual_regs_in_insn > /projects/.../src/gcc/gcc/function.c:1598 > 0x7ffb42 instantiate_virtual_regs > /projects/.../src/gcc/gcc/function.c:1966 > 0x7ffb42 execute > /projects/.../src/gcc/gcc/function.c:2015 > Please submit a full bug report, > with preprocessed source if appropriate. > Please include the complete backtrace with any bug report. > See <http://gcc.gnu.org/bugs.html> for instructions. > > GCC is configured with > > gcc/configure --target=arm-none-linux-gnueabi --prefix= > --with-sysroot=... --enable-shared --disable-libsanitizer > --disable-libssp --disable-libmudflap > --with-plugin-ld=arm-none-linux-gnueabi-ld --enable-checking=yes > --enable-languages=c,c++,fortran --with-gmp=... --with-mpfr=... > --with-mpc=... --with-isl=... --with-cloog=... --with-arch=armv7-a > --with-fpu=vfpv3-d16 --with-float=softfp --with-arch=armv7-a Sorry about that, thought I'd tested that combination. I installed the patch below as obvious after testing on arm-linux-gnu. Thanks, Richard gcc/ * genrecog.c (simplify_tests): Check that CONST_INT and XWINT tests are for the same position. Index: gcc/genrecog.c =================================================================== --- gcc/genrecog.c 2015-04-30 09:06:17.706538299 +0100 +++ gcc/genrecog.c 2015-04-30 12:49:58.689309916 +0100 @@ -1597,7 +1597,8 @@ simplify_tests (state *s) && d->if_statement_p (&label) && label == CONST_INT) if (decision *second = d->first->to->singleton ()) - if (second->test.kind == test::WIDE_INT_FIELD + if (d->test.pos == second->test.pos + && second->test.kind == test::WIDE_INT_FIELD && second->test.u.opno == 0 && second->if_statement_p (&label) && IN_RANGE (int64_t (label), ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford ` (2 preceding siblings ...) 2015-04-30 6:54 ` Bin.Cheng @ 2015-04-30 7:17 ` Andreas Schwab 2015-04-30 7:58 ` Richard Sandiford 2015-05-07 8:59 ` [nvptx] " Thomas Schwinge ` (2 subsequent siblings) 6 siblings, 1 reply; 30+ messages in thread From: Andreas Schwab @ 2015-04-30 7:17 UTC (permalink / raw) To: gcc-patches; +Cc: richard.sandiford Richard Sandiford <richard.sandiford@arm.com> writes: > /* Represents a test and the action that should be taken on the result. > If a transition exists for the test outcome, the machine switches > to the transition's target state. If no suitable transition exists, > the machine either falls through to the next decision or, if there are no > more decisions to try, fails the match. */ > struct decision : list_head <transition> > { > decision (const test &); > > void set_parent (list_head <decision> *s); > bool if_statement_p (uint64_t * = 0) const; > > /* The state to which this decision belongs. */ > state *s; > > /* Links to other decisions in the same state. */ > decision *prev, *next; > > /* The test to perform. */ > struct test test; > }; ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from 'struct test' Bootstrap compiler is gcc 4.3.4. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-30 7:17 ` Andreas Schwab @ 2015-04-30 7:58 ` Richard Sandiford 2015-04-30 12:10 ` Andreas Schwab 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-04-30 7:58 UTC (permalink / raw) To: Andreas Schwab; +Cc: gcc-patches Andreas Schwab <schwab@linux-m68k.org> writes: > Richard Sandiford <richard.sandiford@arm.com> writes: > >> /* Represents a test and the action that should be taken on the result. >> If a transition exists for the test outcome, the machine switches >> to the transition's target state. If no suitable transition exists, >> the machine either falls through to the next decision or, if there are no >> more decisions to try, fails the match. */ >> struct decision : list_head <transition> >> { >> decision (const test &); >> >> void set_parent (list_head <decision> *s); >> bool if_statement_p (uint64_t * = 0) const; >> >> /* The state to which this decision belongs. */ >> state *s; >> >> /* Links to other decisions in the same state. */ >> decision *prev, *next; >> >> /* The test to perform. */ >> struct test test; >> }; > > ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' > ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from 'struct test' > > Bootstrap compiler is gcc 4.3.4. Bah. Does it like "::test test" instead of "struct test test"? Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-30 7:58 ` Richard Sandiford @ 2015-04-30 12:10 ` Andreas Schwab 2015-04-30 12:33 ` Richard Biener 0 siblings, 1 reply; 30+ messages in thread From: Andreas Schwab @ 2015-04-30 12:10 UTC (permalink / raw) To: gcc-patches; +Cc: richard.sandiford Richard Sandiford <richard.sandiford@arm.com> writes: > Andreas Schwab <schwab@linux-m68k.org> writes: >> Richard Sandiford <richard.sandiford@arm.com> writes: >> >>> /* Represents a test and the action that should be taken on the result. >>> If a transition exists for the test outcome, the machine switches >>> to the transition's target state. If no suitable transition exists, >>> the machine either falls through to the next decision or, if there are no >>> more decisions to try, fails the match. */ >>> struct decision : list_head <transition> >>> { >>> decision (const test &); >>> >>> void set_parent (list_head <decision> *s); >>> bool if_statement_p (uint64_t * = 0) const; >>> >>> /* The state to which this decision belongs. */ >>> state *s; >>> >>> /* Links to other decisions in the same state. */ >>> decision *prev, *next; >>> >>> /* The test to perform. */ >>> struct test test; >>> }; >> >> ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' >> ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from 'struct test' >> >> Bootstrap compiler is gcc 4.3.4. > > Bah. Does it like "::test test" instead of "struct test test"? Same error. Andreas. -- Andreas Schwab, schwab@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 "And now for something completely different." ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-30 12:10 ` Andreas Schwab @ 2015-04-30 12:33 ` Richard Biener 2015-04-30 16:27 ` Richard Sandiford 0 siblings, 1 reply; 30+ messages in thread From: Richard Biener @ 2015-04-30 12:33 UTC (permalink / raw) To: Andreas Schwab; +Cc: gcc-patches, richard.sandiford On Thu, Apr 30, 2015 at 2:08 PM, Andreas Schwab <schwab@linux-m68k.org> wrote: > Richard Sandiford <richard.sandiford@arm.com> writes: > >> Andreas Schwab <schwab@linux-m68k.org> writes: >>> Richard Sandiford <richard.sandiford@arm.com> writes: >>> >>>> /* Represents a test and the action that should be taken on the result. >>>> If a transition exists for the test outcome, the machine switches >>>> to the transition's target state. If no suitable transition exists, >>>> the machine either falls through to the next decision or, if there are no >>>> more decisions to try, fails the match. */ >>>> struct decision : list_head <transition> >>>> { >>>> decision (const test &); >>>> >>>> void set_parent (list_head <decision> *s); >>>> bool if_statement_p (uint64_t * = 0) const; >>>> >>>> /* The state to which this decision belongs. */ >>>> state *s; >>>> >>>> /* Links to other decisions in the same state. */ >>>> decision *prev, *next; >>>> >>>> /* The test to perform. */ >>>> struct test test; >>>> }; >>> >>> ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' >>> ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from 'struct test' >>> >>> Bootstrap compiler is gcc 4.3.4. >> >> Bah. Does it like "::test test" instead of "struct test test"? > > Same error. You have to use a different name I believe (or -fpermissive). Richard. > Andreas. > > -- > Andreas Schwab, schwab@linux-m68k.org > GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 > "And now for something completely different." ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-30 12:33 ` Richard Biener @ 2015-04-30 16:27 ` Richard Sandiford 2015-05-01 12:42 ` Richard Sandiford 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-04-30 16:27 UTC (permalink / raw) To: Richard Biener; +Cc: Andreas Schwab, gcc-patches Richard Biener <richard.guenther@gmail.com> writes: > On Thu, Apr 30, 2015 at 2:08 PM, Andreas Schwab <schwab@linux-m68k.org> wrote: >> Richard Sandiford <richard.sandiford@arm.com> writes: >> >>> Andreas Schwab <schwab@linux-m68k.org> writes: >>>> Richard Sandiford <richard.sandiford@arm.com> writes: >>>> >>>>> /* Represents a test and the action that should be taken on the result. >>>>> If a transition exists for the test outcome, the machine switches >>>>> to the transition's target state. If no suitable transition exists, >>>>> the machine either falls through to the next decision or, if there are no >>>>> more decisions to try, fails the match. */ >>>>> struct decision : list_head <transition> >>>>> { >>>>> decision (const test &); >>>>> >>>>> void set_parent (list_head <decision> *s); >>>>> bool if_statement_p (uint64_t * = 0) const; >>>>> >>>>> /* The state to which this decision belongs. */ >>>>> state *s; >>>>> >>>>> /* Links to other decisions in the same state. */ >>>>> decision *prev, *next; >>>>> >>>>> /* The test to perform. */ >>>>> struct test test; >>>>> }; >>>> >>>> ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' >>>> ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from >>>> struct test' >>>> >>>> Bootstrap compiler is gcc 4.3.4. >>> >>> Bah. Does it like "::test test" instead of "struct test test"? >> >> Same error. > > You have to use a different name I believe (or -fpermissive). Hmm, but then why does it work with more recent compilers? Thanks, Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-30 16:27 ` Richard Sandiford @ 2015-05-01 12:42 ` Richard Sandiford 2015-05-01 13:57 ` Jeff Law 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-05-01 12:42 UTC (permalink / raw) To: Richard Biener; +Cc: Andreas Schwab, gcc-patches Richard Sandiford <richard.sandiford@arm.com> writes: > Richard Biener <richard.guenther@gmail.com> writes: >> On Thu, Apr 30, 2015 at 2:08 PM, Andreas Schwab <schwab@linux-m68k.org> wrote: >>> Richard Sandiford <richard.sandiford@arm.com> writes: >>> >>>> Andreas Schwab <schwab@linux-m68k.org> writes: >>>>> Richard Sandiford <richard.sandiford@arm.com> writes: >>>>> >>>>>> /* Represents a test and the action that should be taken on the result. >>>>>> If a transition exists for the test outcome, the machine switches >>>>>> to the transition's target state. If no suitable transition exists, >>>>>> the machine either falls through to the next decision or, if there are no >>>>>> more decisions to try, fails the match. */ >>>>>> struct decision : list_head <transition> >>>>>> { >>>>>> decision (const test &); >>>>>> >>>>>> void set_parent (list_head <decision> *s); >>>>>> bool if_statement_p (uint64_t * = 0) const; >>>>>> >>>>>> /* The state to which this decision belongs. */ >>>>>> state *s; >>>>>> >>>>>> /* Links to other decisions in the same state. */ >>>>>> decision *prev, *next; >>>>>> >>>>>> /* The test to perform. */ >>>>>> struct test test; >>>>>> }; >>>>> >>>>> ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' >>>>> ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from >>>>> struct test' >>>>> >>>>> Bootstrap compiler is gcc 4.3.4. >>>> >>>> Bah. Does it like "::test test" instead of "struct test test"? >>> >>> Same error. >> >> You have to use a different name I believe (or -fpermissive). > > Hmm, but then why does it work with more recent compilers? Whatever the reason, I suppose the path of least resistance is to rename the thing. Tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * genrecog.c (test): Rename to rtx_test. Update rest of file accordingly. Index: gcc/genrecog.c =================================================================== --- gcc/genrecog.c 2015-04-30 17:45:29.662231979 +0100 +++ gcc/genrecog.c 2015-04-30 17:45:29.806230258 +0100 @@ -1047,9 +1047,9 @@ struct pattern_use }; /* Represents a test performed by a decision. */ -struct test +struct rtx_test { - test (); + rtx_test (); /* The types of test that can be performed. Most of them take as input an rtx X. Some also take as input a transition label LABEL; the others @@ -1140,136 +1140,136 @@ struct test acceptance_type acceptance; } u; - static test code (position *); - static test mode (position *); - static test int_field (position *, int); - static test wide_int_field (position *, int); - static test veclen (position *); - static test peep2_count (int); - static test veclen_ge (position *, int); - static test predicate (position *, const pred_data *, machine_mode); - static test duplicate (position *, int); - static test pattern (position *, pattern_use *); - static test have_num_clobbers (); - static test c_test (const char *); - static test set_op (position *, int); - static test accept (const acceptance_type &); + static rtx_test code (position *); + static rtx_test mode (position *); + static rtx_test int_field (position *, int); + static rtx_test wide_int_field (position *, int); + static rtx_test veclen (position *); + static rtx_test peep2_count (int); + static rtx_test veclen_ge (position *, int); + static rtx_test predicate (position *, const pred_data *, machine_mode); + static rtx_test duplicate (position *, int); + static rtx_test pattern (position *, pattern_use *); + static rtx_test have_num_clobbers (); + static rtx_test c_test (const char *); + static rtx_test set_op (position *, int); + static rtx_test accept (const acceptance_type &); bool terminal_p () const; bool single_outcome_p () const; private: - test (position *, kind_enum); + rtx_test (position *, kind_enum); }; -test::test () {} +rtx_test::rtx_test () {} -test::test (position *pos_in, kind_enum kind_in) +rtx_test::rtx_test (position *pos_in, kind_enum kind_in) : pos (pos_in), pos_operand (-1), kind (kind_in) {} -test -test::code (position *pos) +rtx_test +rtx_test::code (position *pos) { - return test (pos, test::CODE); + return rtx_test (pos, rtx_test::CODE); } -test -test::mode (position *pos) +rtx_test +rtx_test::mode (position *pos) { - return test (pos, test::MODE); + return rtx_test (pos, rtx_test::MODE); } -test -test::int_field (position *pos, int opno) +rtx_test +rtx_test::int_field (position *pos, int opno) { - test res (pos, test::INT_FIELD); + rtx_test res (pos, rtx_test::INT_FIELD); res.u.opno = opno; return res; } -test -test::wide_int_field (position *pos, int opno) +rtx_test +rtx_test::wide_int_field (position *pos, int opno) { - test res (pos, test::WIDE_INT_FIELD); + rtx_test res (pos, rtx_test::WIDE_INT_FIELD); res.u.opno = opno; return res; } -test -test::veclen (position *pos) +rtx_test +rtx_test::veclen (position *pos) { - return test (pos, test::VECLEN); + return rtx_test (pos, rtx_test::VECLEN); } -test -test::peep2_count (int min_len) +rtx_test +rtx_test::peep2_count (int min_len) { - test res (0, test::PEEP2_COUNT); + rtx_test res (0, rtx_test::PEEP2_COUNT); res.u.min_len = min_len; return res; } -test -test::veclen_ge (position *pos, int min_len) +rtx_test +rtx_test::veclen_ge (position *pos, int min_len) { - test res (pos, test::VECLEN_GE); + rtx_test res (pos, rtx_test::VECLEN_GE); res.u.min_len = min_len; return res; } -test -test::predicate (position *pos, const struct pred_data *data, - machine_mode mode) +rtx_test +rtx_test::predicate (position *pos, const struct pred_data *data, + machine_mode mode) { - test res (pos, test::PREDICATE); + rtx_test res (pos, rtx_test::PREDICATE); res.u.predicate.data = data; res.u.predicate.mode_is_param = false; res.u.predicate.mode = mode; return res; } -test -test::duplicate (position *pos, int opno) +rtx_test +rtx_test::duplicate (position *pos, int opno) { - test res (pos, test::DUPLICATE); + rtx_test res (pos, rtx_test::DUPLICATE); res.u.opno = opno; return res; } -test -test::pattern (position *pos, pattern_use *pattern) +rtx_test +rtx_test::pattern (position *pos, pattern_use *pattern) { - test res (pos, test::PATTERN); + rtx_test res (pos, rtx_test::PATTERN); res.u.pattern = pattern; return res; } -test -test::have_num_clobbers () +rtx_test +rtx_test::have_num_clobbers () { - return test (0, test::HAVE_NUM_CLOBBERS); + return rtx_test (0, rtx_test::HAVE_NUM_CLOBBERS); } -test -test::c_test (const char *string) +rtx_test +rtx_test::c_test (const char *string) { - test res (0, test::C_TEST); + rtx_test res (0, rtx_test::C_TEST); res.u.string = string; return res; } -test -test::set_op (position *pos, int opno) +rtx_test +rtx_test::set_op (position *pos, int opno) { - test res (pos, test::SET_OP); + rtx_test res (pos, rtx_test::SET_OP); res.u.opno = opno; return res; } -test -test::accept (const acceptance_type &acceptance) +rtx_test +rtx_test::accept (const acceptance_type &acceptance) { - test res (0, test::ACCEPT); + rtx_test res (0, rtx_test::ACCEPT); res.u.acceptance = acceptance; return res; } @@ -1277,66 +1277,66 @@ test::accept (const acceptance_type &acc /* Return true if the test represents an unconditionally successful match. */ bool -test::terminal_p () const +rtx_test::terminal_p () const { - return kind == test::ACCEPT && u.acceptance.type != PEEPHOLE2; + return kind == rtx_test::ACCEPT && u.acceptance.type != PEEPHOLE2; } /* Return true if the test is a boolean that is always true. */ bool -test::single_outcome_p () const +rtx_test::single_outcome_p () const { - return terminal_p () || kind == test::SET_OP; + return terminal_p () || kind == rtx_test::SET_OP; } bool -operator == (const test &a, const test &b) +operator == (const rtx_test &a, const rtx_test &b) { if (a.pos != b.pos || a.kind != b.kind) return false; switch (a.kind) { - case test::CODE: - case test::MODE: - case test::VECLEN: - case test::HAVE_NUM_CLOBBERS: + case rtx_test::CODE: + case rtx_test::MODE: + case rtx_test::VECLEN: + case rtx_test::HAVE_NUM_CLOBBERS: return true; - case test::PEEP2_COUNT: - case test::VECLEN_GE: + case rtx_test::PEEP2_COUNT: + case rtx_test::VECLEN_GE: return a.u.min_len == b.u.min_len; - case test::INT_FIELD: - case test::WIDE_INT_FIELD: - case test::DUPLICATE: - case test::SET_OP: + case rtx_test::INT_FIELD: + case rtx_test::WIDE_INT_FIELD: + case rtx_test::DUPLICATE: + case rtx_test::SET_OP: return a.u.opno == b.u.opno; - case test::SAVED_CONST_INT: + case rtx_test::SAVED_CONST_INT: return (a.u.integer.is_param == b.u.integer.is_param && a.u.integer.value == b.u.integer.value); - case test::PREDICATE: + case rtx_test::PREDICATE: return (a.u.predicate.data == b.u.predicate.data && a.u.predicate.mode_is_param == b.u.predicate.mode_is_param && a.u.predicate.mode == b.u.predicate.mode); - case test::PATTERN: + case rtx_test::PATTERN: return (a.u.pattern->routine == b.u.pattern->routine && a.u.pattern->params == b.u.pattern->params); - case test::C_TEST: + case rtx_test::C_TEST: return strcmp (a.u.string, b.u.string) == 0; - case test::ACCEPT: + case rtx_test::ACCEPT: return a.u.acceptance == b.u.acceptance; } gcc_unreachable (); } bool -operator != (const test &a, const test &b) +operator != (const rtx_test &a, const rtx_test &b) { return !operator == (a, b); } @@ -1420,8 +1420,8 @@ struct transition transition *prev, *next; /* The transition should be taken when T has one of these values. - E.g. for test::CODE this is a set of codes, while for booleans like - test::PREDICATE it is always a singleton "true". The labels are + E.g. for rtx_test::CODE this is a set of codes, while for booleans like + rtx_test::PREDICATE it is always a singleton "true". The labels are sorted in ascending order. */ int_set labels; @@ -1439,7 +1439,7 @@ struct transition bool optional; /* True if LABELS contains parameter numbers rather than constants. - E.g. if this is true for a test::CODE, the label is the number + E.g. if this is true for a rtx_test::CODE, the label is the number of an rtx_code parameter rather than an rtx_code itself. LABELS is always a singleton when this variable is true. */ bool is_param; @@ -1452,7 +1452,7 @@ struct transition more decisions to try, fails the match. */ struct decision : list_head <transition> { - decision (const test &); + decision (const rtx_test &); void set_parent (list_head <decision> *s); bool if_statement_p (uint64_t * = 0) const; @@ -1464,7 +1464,7 @@ struct decision : list_head <transition> decision *prev, *next; /* The test to perform. */ - struct test test; + rtx_test test; }; /* Represents one machine state. For each state the machine tries a list @@ -1488,7 +1488,7 @@ transition::set_parent (list_head <trans from = static_cast <decision *> (from_in); } -decision::decision (const struct test &test_in) +decision::decision (const rtx_test &test_in) : prev (0), next (0), test (test_in) {} /* Set the state to which this decision belongs. */ @@ -1518,7 +1518,7 @@ decision::if_statement_p (uint64_t *labe TRANS. */ static void -add_decision (state *from, const test &test, transition *trans) +add_decision (state *from, const rtx_test &test, transition *trans) { decision *d = new decision (test); from->push_back (d); @@ -1530,7 +1530,7 @@ add_decision (state *from, const test &t should be optional. Return the new state. */ static state * -add_decision (state *from, const test &test, int_set labels, bool optional) +add_decision (state *from, const rtx_test &test, int_set labels, bool optional) { state *to = new state; add_decision (from, test, new transition (labels, to, optional)); @@ -1542,7 +1542,7 @@ add_decision (state *from, const test &t optional. */ static decision * -insert_decision_before (state::range r, const test &test, +insert_decision_before (state::range r, const rtx_test &test, const int_set &labels, bool optional) { decision *newd = new decision (test); @@ -1593,18 +1593,18 @@ simplify_tests (state *s) uint64_t label; /* Convert checks for GET_CODE (x) == CONST_INT and XWINT (x, 0) == N into checks for const_int_rtx[N'], if N is suitably small. */ - if (d->test.kind == test::CODE + if (d->test.kind == rtx_test::CODE && d->if_statement_p (&label) && label == CONST_INT) if (decision *second = d->first->to->singleton ()) if (d->test.pos == second->test.pos - && second->test.kind == test::WIDE_INT_FIELD + && second->test.kind == rtx_test::WIDE_INT_FIELD && second->test.u.opno == 0 && second->if_statement_p (&label) && IN_RANGE (int64_t (label), -MAX_SAVED_CONST_INT, MAX_SAVED_CONST_INT)) { - d->test.kind = test::SAVED_CONST_INT; + d->test.kind = rtx_test::SAVED_CONST_INT; d->test.u.integer.is_param = false; d->test.u.integer.value = label; d->replace (d->first, second->release ()); @@ -1619,7 +1619,7 @@ simplify_tests (state *s) paths that reach that code test require the same predicate to be true. cse_tests will then put the predicate test in series with the code test. */ - if (d->test.kind == test::CODE) + if (d->test.kind == rtx_test::CODE) if (transition *trans = d->singleton ()) { state *s = trans->to; @@ -1630,7 +1630,7 @@ simplify_tests (state *s) transition *trans2 = d2->singleton (); if (!trans2) break; - if (d2->test.kind == test::PREDICATE) + if (d2->test.kind == rtx_test::PREDICATE) { d->test = d2->test; trans->labels = int_set (true); @@ -1654,7 +1654,7 @@ simplify_tests (state *s) static bool common_test_p (decision *d, transition *common, vec <transition *> *where) { - if (d->test.kind == test::ACCEPT) + if (d->test.kind == rtx_test::ACCEPT) /* We found a successful return that didn't require COMMON. */ return false; if (d->test == common->from->test) @@ -1702,20 +1702,20 @@ struct known_conditions as positive proof. */ static bool -safe_to_hoist_p (decision *d, const test &test, known_conditions *kc) +safe_to_hoist_p (decision *d, const rtx_test &test, known_conditions *kc) { switch (test.kind) { - case test::C_TEST: + case rtx_test::C_TEST: /* In general, C tests require everything else to have been verified and all operands to have been set up. */ return false; - case test::ACCEPT: + case rtx_test::ACCEPT: /* Don't accept something before all conditions have been tested. */ return false; - case test::PREDICATE: + case rtx_test::PREDICATE: /* Don't move a predicate over a test for VECLEN_GE, since the predicate used in a match_parallel can legitimately expect the length to be checked first. */ @@ -1723,21 +1723,21 @@ safe_to_hoist_p (decision *d, const test subd->test != test; subd = subd->first->to->first) if (subd->test.pos == test.pos - && subd->test.kind == test::VECLEN_GE) + && subd->test.kind == rtx_test::VECLEN_GE) return false; goto any_rtx; - case test::DUPLICATE: + case rtx_test::DUPLICATE: /* Don't test for a match_dup until the associated operand has been set. */ if (!kc->set_operands[test.u.opno]) return false; goto any_rtx; - case test::CODE: - case test::MODE: - case test::SAVED_CONST_INT: - case test::SET_OP: + case rtx_test::CODE: + case rtx_test::MODE: + case rtx_test::SAVED_CONST_INT: + case rtx_test::SET_OP: any_rtx: /* Check whether it is safe to access the rtx under test. */ switch (test.pos->type) @@ -1753,20 +1753,20 @@ safe_to_hoist_p (decision *d, const test } gcc_unreachable (); - case test::INT_FIELD: - case test::WIDE_INT_FIELD: - case test::VECLEN: - case test::VECLEN_GE: + case rtx_test::INT_FIELD: + case rtx_test::WIDE_INT_FIELD: + case rtx_test::VECLEN: + case rtx_test::VECLEN_GE: /* These tests access a specific part of an rtx, so are only safe once we know what the rtx is. */ return kc->position_tests[test.pos->id] & TESTED_CODE; - case test::PEEP2_COUNT: - case test::HAVE_NUM_CLOBBERS: + case rtx_test::PEEP2_COUNT: + case rtx_test::HAVE_NUM_CLOBBERS: /* These tests can be performed anywhere. */ return true; - case test::PATTERN: + case rtx_test::PATTERN: gcc_unreachable (); } gcc_unreachable (); @@ -1887,8 +1887,8 @@ cse_tests (position *pos, state *s, know /* Make sure that safe_to_hoist_p isn't being overly conservative. It should realize that D's test is safe in the current environment. */ - gcc_assert (d->test.kind == test::C_TEST - || d->test.kind == test::ACCEPT + gcc_assert (d->test.kind == rtx_test::C_TEST + || d->test.kind == rtx_test::ACCEPT || safe_to_hoist_p (d, d->test, kc)); /* D won't be changed any further by the current optimization. @@ -1896,24 +1896,24 @@ cse_tests (position *pos, state *s, know int prev = 0; switch (d->test.kind) { - case test::CODE: + case rtx_test::CODE: prev = kc->position_tests[d->test.pos->id]; kc->position_tests[d->test.pos->id] |= TESTED_CODE; break; - case test::VECLEN: - case test::VECLEN_GE: + case rtx_test::VECLEN: + case rtx_test::VECLEN_GE: prev = kc->position_tests[d->test.pos->id]; kc->position_tests[d->test.pos->id] |= TESTED_VECLEN; break; - case test::SET_OP: + case rtx_test::SET_OP: prev = kc->set_operands[d->test.u.opno]; gcc_assert (!prev); kc->set_operands[d->test.u.opno] = true; break; - case test::PEEP2_COUNT: + case rtx_test::PEEP2_COUNT: prev = kc->peep2_count; kc->peep2_count = MAX (prev, d->test.u.min_len); break; @@ -1925,17 +1925,17 @@ cse_tests (position *pos, state *s, know cse_tests (d->test.pos ? d->test.pos : pos, trans->to, kc); switch (d->test.kind) { - case test::CODE: - case test::VECLEN: - case test::VECLEN_GE: + case rtx_test::CODE: + case rtx_test::VECLEN: + case rtx_test::VECLEN_GE: kc->position_tests[d->test.pos->id] = prev; break; - case test::SET_OP: + case rtx_test::SET_OP: kc->set_operands[d->test.u.opno] = prev; break; - case test::PEEP2_COUNT: + case rtx_test::PEEP2_COUNT: kc->peep2_count = prev; break; @@ -1949,33 +1949,33 @@ cse_tests (position *pos, state *s, know or parameter::UNSET if none. */ parameter::type_enum -transition_parameter_type (test::kind_enum kind) +transition_parameter_type (rtx_test::kind_enum kind) { switch (kind) { - case test::CODE: + case rtx_test::CODE: return parameter::CODE; - case test::MODE: + case rtx_test::MODE: return parameter::MODE; - case test::INT_FIELD: - case test::VECLEN: - case test::PATTERN: + case rtx_test::INT_FIELD: + case rtx_test::VECLEN: + case rtx_test::PATTERN: return parameter::INT; - case test::WIDE_INT_FIELD: + case rtx_test::WIDE_INT_FIELD: return parameter::WIDE_INT; - case test::PEEP2_COUNT: - case test::VECLEN_GE: - case test::SAVED_CONST_INT: - case test::PREDICATE: - case test::DUPLICATE: - case test::HAVE_NUM_CLOBBERS: - case test::C_TEST: - case test::SET_OP: - case test::ACCEPT: + case rtx_test::PEEP2_COUNT: + case rtx_test::VECLEN_GE: + case rtx_test::SAVED_CONST_INT: + case rtx_test::PREDICATE: + case rtx_test::DUPLICATE: + case rtx_test::HAVE_NUM_CLOBBERS: + case rtx_test::C_TEST: + case rtx_test::SET_OP: + case rtx_test::ACCEPT: return parameter::UNSET; } gcc_unreachable (); @@ -1993,11 +1993,11 @@ find_operand_positions (state *s, vec <i int this_operand = (d->test.pos ? operand_pos[d->test.pos->id] : -1); if (this_operand >= 0) d->test.pos_operand = this_operand; - if (d->test.kind == test::SET_OP) + if (d->test.kind == rtx_test::SET_OP) operand_pos[d->test.pos->id] = d->test.u.opno; for (transition *trans = d->first; trans; trans = trans->next) find_operand_positions (trans->to, operand_pos); - if (d->test.kind == test::SET_OP) + if (d->test.kind == rtx_test::SET_OP) operand_pos[d->test.pos->id] = this_operand; } } @@ -2065,7 +2065,7 @@ get_stats (state *s) for_d.num_decisions += 1; for_d.longest_path += 1; } - if (d->test.kind == test::ACCEPT) + if (d->test.kind == rtx_test::ACCEPT) { for_d.longest_path_code = d->test.u.acceptance.u.full.code; for_d.longest_backtrack_code = d->test.u.acceptance.u.full.code; @@ -2379,21 +2379,21 @@ update_parameters (vec <parameter> &to, PARAMB alone. */ static bool -compatible_tests_p (const test &a, const test &b, +compatible_tests_p (const rtx_test &a, const rtx_test &b, parameter *parama, parameter *paramb) { if (a.kind != b.kind) return false; switch (a.kind) { - case test::PREDICATE: + case rtx_test::PREDICATE: if (a.u.predicate.data != b.u.predicate.data) return false; *parama = parameter (parameter::MODE, false, a.u.predicate.mode); *paramb = parameter (parameter::MODE, false, b.u.predicate.mode); return true; - case test::SAVED_CONST_INT: + case rtx_test::SAVED_CONST_INT: *parama = parameter (parameter::INT, false, a.u.integer.value); *paramb = parameter (parameter::INT, false, b.u.integer.value); return true; @@ -2667,7 +2667,7 @@ merge_patterns (merge_state_info *sinfo1 parameterizing the first N const_ints of the vector and then (once we reach the maximum number of parameters) we go on to match the other elements exactly. */ - if (d1->test.kind == test::WIDE_INT_FIELD) + if (d1->test.kind == rtx_test::WIDE_INT_FIELD) return false; /* See whether the label has a generalizable type. */ @@ -2803,7 +2803,7 @@ init_pattern_use (create_pattern_info *c pattern_use *use = new pattern_use; use->routine = pat->routine; use->params.splice (params); - decision *d = new decision (test::pattern (res->root, use)); + decision *d = new decision (rtx_test::pattern (res->root, use)); /* If the original decision could use an element of operands[] instead of an rtx variable, try to transfer it to the new decision. */ @@ -2823,7 +2823,7 @@ add_pattern_acceptance (create_pattern_i acceptance.type = SUBPATTERN; acceptance.partial_p = false; acceptance.u.full.code = cpi->next_result; - add_decision (s, test::accept (acceptance), true, false); + add_decision (s, rtx_test::accept (acceptance), true, false); cpi->next_result += 1; } @@ -2860,12 +2860,12 @@ populate_pattern_routine (create_pattern const parameter ¶m = params[pat->param_test]; switch (newd->test.kind) { - case test::PREDICATE: + case rtx_test::PREDICATE: newd->test.u.predicate.mode_is_param = param.is_param; newd->test.u.predicate.mode = param.value; break; - case test::SAVED_CONST_INT: + case rtx_test::SAVED_CONST_INT: newd->test.u.integer.is_param = param.is_param; newd->test.u.integer.value = param.value; break; @@ -2875,9 +2875,9 @@ populate_pattern_routine (create_pattern break; } } - if (d->test.kind == test::C_TEST) + if (d->test.kind == rtx_test::C_TEST) routine->insn_p = true; - else if (d->test.kind == test::HAVE_NUM_CLOBBERS) + else if (d->test.kind == rtx_test::HAVE_NUM_CLOBBERS) routine->pnum_clobbers_p = true; news->push_back (newd); @@ -3038,11 +3038,11 @@ split_out_patterns (vec <merge_state_inf and so couldn't be shared between states). */ if (decision *d = sinfo->s->singleton ()) /* ACCEPT states are unique, so don't even try to merge them. */ - if (d->test.kind != test::ACCEPT + if (d->test.kind != rtx_test::ACCEPT && (pattern_have_num_clobbers_p - || d->test.kind != test::HAVE_NUM_CLOBBERS) + || d->test.kind != rtx_test::HAVE_NUM_CLOBBERS) && (pattern_c_test_p - || d->test.kind != test::C_TEST)) + || d->test.kind != rtx_test::C_TEST)) { merge_state_info **slot = hashtab.find_slot (sinfo, INSERT); sinfo->prev_same_test = *slot; @@ -3290,7 +3290,7 @@ create_subroutine (routine_type type, st acceptance.partial_p = true; acceptance.u.subroutine_id = procs.length (); state *news = new state; - add_decision (news, test::accept (acceptance), true, false); + add_decision (news, rtx_test::accept (acceptance), true, false); return news; } @@ -3327,8 +3327,8 @@ find_subroutines (routine_type type, sta if (!newd->test.single_outcome_p ()) size.num_statements += 1; trans = newd->singleton (); - if (newd->test.kind == test::SET_OP - || newd->test.kind == test::ACCEPT) + if (newd->test.kind == rtx_test::SET_OP + || newd->test.kind == rtx_test::ACCEPT) break; } /* The target of TRANS is a subroutine candidate. First recurse @@ -3866,9 +3866,9 @@ match_pattern_2 (state *s, rtx top_patte if (code == MATCH_PARALLEL || code == MATCH_PAR_DUP) { /* Check that we have a parallel with enough elements. */ - s = add_decision (s, test::code (pos), PARALLEL, false); + s = add_decision (s, rtx_test::code (pos), PARALLEL, false); int min_len = XVECLEN (pattern, 2); - s = add_decision (s, test::veclen_ge (pos, min_len), + s = add_decision (s, rtx_test::veclen_ge (pos, min_len), true, false); } else @@ -3883,7 +3883,7 @@ match_pattern_2 (state *s, rtx top_patte bool need_codes = (pred && (code == MATCH_OPERATOR || code == MATCH_OP_DUP)); - s = add_decision (s, test::code (pos), codes, !need_codes); + s = add_decision (s, rtx_test::code (pos), codes, !need_codes); } /* Postpone the predicate check until we've checked the rest @@ -3924,7 +3924,7 @@ match_pattern_2 (state *s, rtx top_patte default: { /* Check that the rtx has the right code. */ - s = add_decision (s, test::code (pos), code, false); + s = add_decision (s, rtx_test::code (pos), code, false); /* Queue a test for the mode if one is specified. */ if (GET_MODE (pattern) != VOIDmode) @@ -3949,7 +3949,8 @@ match_pattern_2 (state *s, rtx top_patte /* Make sure the vector has the right number of elements. */ int length = XVECLEN (pattern, i); - s = add_decision (s, test::veclen (pos), length, false); + s = add_decision (s, rtx_test::veclen (pos), + length, false); position **subpos2_ptr = &pos->xvecexp0s; for (int j = 0; j < length; j++) @@ -3965,13 +3966,13 @@ match_pattern_2 (state *s, rtx top_patte case 'i': /* Make sure that XINT (X, I) has the right value. */ - s = add_decision (s, test::int_field (pos, i), + s = add_decision (s, rtx_test::int_field (pos, i), XINT (pattern, i), false); break; case 'w': /* Make sure that XWINT (X, I) has the right value. */ - s = add_decision (s, test::wide_int_field (pos, i), + s = add_decision (s, rtx_test::wide_int_field (pos, i), XWINT (pattern, 0), false); break; @@ -4037,7 +4038,7 @@ match_pattern_2 (state *s, rtx top_patte and DImode register_operands, as described above. */ machine_mode mode = GET_MODE (e->pattern); if (safe_predicate_mode (pred, mode)) - s = add_decision (s, test::mode (e->pos), mode, true); + s = add_decision (s, rtx_test::mode (e->pos), mode, true); /* Assign to operands[] first, so that the rtx usually doesn't need to be live across the call to the predicate. @@ -4046,19 +4047,21 @@ match_pattern_2 (state *s, rtx top_patte since we fully expect to assign to operands[] at some point, and since the caller usually writes to other parts of recog_data anyway. */ - s = add_decision (s, test::set_op (e->pos, opno), true, false); - s = add_decision (s, test::predicate (e->pos, pred, mode), + s = add_decision (s, rtx_test::set_op (e->pos, opno), + true, false); + s = add_decision (s, rtx_test::predicate (e->pos, pred, mode), true, false); } else /* Historically we've ignored the mode when there's no predicate. Just set up operands[] unconditionally. */ - s = add_decision (s, test::set_op (e->pos, opno), true, false); + s = add_decision (s, rtx_test::set_op (e->pos, opno), + true, false); break; } default: - s = add_decision (s, test::mode (e->pos), + s = add_decision (s, rtx_test::mode (e->pos), GET_MODE (e->pattern), false); break; } @@ -4066,7 +4069,7 @@ match_pattern_2 (state *s, rtx top_patte /* Finally add rtx_equal_p checks for duplicated operands. */ FOR_EACH_VEC_ELT (dup_tests, i, e) - s = add_decision (s, test::duplicate (e->pos, XINT (e->pattern, 0)), + s = add_decision (s, rtx_test::duplicate (e->pos, XINT (e->pattern, 0)), true, false); return s; } @@ -4098,7 +4101,7 @@ match_pattern_1 (state *s, rtx top_patte position *subpos = next_position (subpos_ptr, &root_pos, POS_PEEP2_INSN, count); if (count > 0) - s = add_decision (s, test::peep2_count (count + 1), + s = add_decision (s, rtx_test::peep2_count (count + 1), true, false); s = match_pattern_2 (s, top_pattern, subpos, x); subpos_ptr = &subpos->next; @@ -4115,15 +4118,15 @@ match_pattern_1 (state *s, rtx top_patte /* If the match is only valid when extra clobbers are added, make sure we're able to pass that information to the caller. */ if (acceptance.type == RECOG && acceptance.u.full.u.num_clobbers) - s = add_decision (s, test::have_num_clobbers (), true, false); + s = add_decision (s, rtx_test::have_num_clobbers (), true, false); } /* Make sure that the C test is true. */ if (maybe_eval_c_test (c_test) != 1) - s = add_decision (s, test::c_test (c_test), true, false); + s = add_decision (s, rtx_test::c_test (c_test), true, false); /* Accept the pattern. */ - add_decision (s, test::accept (acceptance), true, false); + add_decision (s, rtx_test::accept (acceptance), true, false); } /* Like match_pattern_1, but (if merge_states_p) try to merge the @@ -4321,7 +4324,7 @@ struct output_state terminal_pattern_p (decision *d, unsigned int *base_out, unsigned int *count_out) { - if (d->test.kind != test::PATTERN) + if (d->test.kind != rtx_test::PATTERN) return false; unsigned int base = 0; unsigned int count = 0; @@ -4330,7 +4333,7 @@ terminal_pattern_p (decision *d, unsigne if (trans->is_param || trans->labels.length () != 1) return false; decision *subd = trans->to->singleton (); - if (!subd || subd->test.kind != test::ACCEPT) + if (!subd || subd->test.kind != rtx_test::ACCEPT) return false; unsigned int this_base = (subd->test.u.acceptance.u.full.code - trans->labels[0]); @@ -4349,7 +4352,7 @@ terminal_pattern_p (decision *d, unsigne already available in state OS. */ static bool -test_position_available_p (output_state *os, const test &test) +test_position_available_p (output_state *os, const rtx_test &test) { return (!test.pos || test.pos_operand >= 0 @@ -4461,7 +4464,7 @@ print_parameter_value (const parameter & /* Print the C expression for the rtx tested by TEST. */ static void -print_test_rtx (output_state *os, const test &test) +print_test_rtx (output_state *os, const rtx_test &test) { if (test.pos_operand >= 0) printf ("operands[%d]", test.pos_operand); @@ -4472,41 +4475,41 @@ print_test_rtx (output_state *os, const /* Print the C expression for non-boolean test TEST. */ static void -print_nonbool_test (output_state *os, const test &test) +print_nonbool_test (output_state *os, const rtx_test &test) { switch (test.kind) { - case test::CODE: + case rtx_test::CODE: printf ("GET_CODE ("); print_test_rtx (os, test); printf (")"); break; - case test::MODE: + case rtx_test::MODE: printf ("GET_MODE ("); print_test_rtx (os, test); printf (")"); break; - case test::VECLEN: + case rtx_test::VECLEN: printf ("XVECLEN ("); print_test_rtx (os, test); printf (", 0)"); break; - case test::INT_FIELD: + case rtx_test::INT_FIELD: printf ("XINT ("); print_test_rtx (os, test); printf (", %d)", test.u.opno); break; - case test::WIDE_INT_FIELD: + case rtx_test::WIDE_INT_FIELD: printf ("XWINT ("); print_test_rtx (os, test); printf (", %d)", test.u.opno); break; - case test::PATTERN: + case rtx_test::PATTERN: { pattern_routine *routine = test.u.pattern->routine; printf ("pattern%d (", routine->pattern_id); @@ -4536,15 +4539,15 @@ print_nonbool_test (output_state *os, co break; } - case test::PEEP2_COUNT: - case test::VECLEN_GE: - case test::SAVED_CONST_INT: - case test::DUPLICATE: - case test::PREDICATE: - case test::SET_OP: - case test::HAVE_NUM_CLOBBERS: - case test::C_TEST: - case test::ACCEPT: + case rtx_test::PEEP2_COUNT: + case rtx_test::VECLEN_GE: + case rtx_test::SAVED_CONST_INT: + case rtx_test::DUPLICATE: + case rtx_test::PREDICATE: + case rtx_test::SET_OP: + case rtx_test::HAVE_NUM_CLOBBERS: + case rtx_test::C_TEST: + case rtx_test::ACCEPT: gcc_unreachable (); } } @@ -4553,7 +4556,7 @@ print_nonbool_test (output_state *os, co decision performs TEST. Print the C code for the label. */ static void -print_label_value (const test &test, bool is_param, uint64_t value) +print_label_value (const rtx_test &test, bool is_param, uint64_t value) { print_parameter_value (parameter (transition_parameter_type (test.kind), is_param, value)); @@ -4564,24 +4567,24 @@ print_label_value (const test &test, boo Test for inequality if INVERT_P, otherwise test for equality. */ static void -print_test (output_state *os, const test &test, bool is_param, uint64_t value, - bool invert_p) +print_test (output_state *os, const rtx_test &test, bool is_param, + uint64_t value, bool invert_p) { switch (test.kind) { /* Handle the non-boolean TESTs. */ - case test::CODE: - case test::MODE: - case test::VECLEN: - case test::INT_FIELD: - case test::WIDE_INT_FIELD: - case test::PATTERN: + case rtx_test::CODE: + case rtx_test::MODE: + case rtx_test::VECLEN: + case rtx_test::INT_FIELD: + case rtx_test::WIDE_INT_FIELD: + case rtx_test::PATTERN: print_nonbool_test (os, test); printf (" %s ", invert_p ? "!=" : "=="); print_label_value (test, is_param, value); break; - case test::SAVED_CONST_INT: + case rtx_test::SAVED_CONST_INT: gcc_assert (!is_param && value == 1); print_test_rtx (os, test); printf (" %s const_int_rtx[MAX_SAVED_CONST_INT + ", @@ -4592,20 +4595,20 @@ print_test (output_state *os, const test printf ("]"); break; - case test::PEEP2_COUNT: + case rtx_test::PEEP2_COUNT: gcc_assert (!is_param && value == 1); printf ("peep2_current_count %s %d", invert_p ? "<" : ">=", test.u.min_len); break; - case test::VECLEN_GE: + case rtx_test::VECLEN_GE: gcc_assert (!is_param && value == 1); printf ("XVECLEN ("); print_test_rtx (os, test); printf (", 0) %s %d", invert_p ? "<" : ">=", test.u.min_len); break; - case test::PREDICATE: + case rtx_test::PREDICATE: gcc_assert (!is_param && value == 1); printf ("%s%s (", invert_p ? "!" : "", test.u.predicate.data->name); print_test_rtx (os, test); @@ -4616,27 +4619,27 @@ print_test (output_state *os, const test printf (")"); break; - case test::DUPLICATE: + case rtx_test::DUPLICATE: gcc_assert (!is_param && value == 1); printf ("%srtx_equal_p (", invert_p ? "!" : ""); print_test_rtx (os, test); printf (", operands[%d])", test.u.opno); break; - case test::HAVE_NUM_CLOBBERS: + case rtx_test::HAVE_NUM_CLOBBERS: gcc_assert (!is_param && value == 1); printf ("pnum_clobbers %s NULL", invert_p ? "==" : "!="); break; - case test::C_TEST: + case rtx_test::C_TEST: gcc_assert (!is_param && value == 1); if (invert_p) printf ("!"); print_c_condition (test.u.string); break; - case test::ACCEPT: - case test::SET_OP: + case rtx_test::ACCEPT: + case rtx_test::SET_OP: gcc_unreachable (); } } @@ -4812,9 +4815,9 @@ print_decision (output_state *os, decisi return ES_FALLTHROUGH; } } - else if (d->test.kind == test::ACCEPT) + else if (d->test.kind == rtx_test::ACCEPT) return print_acceptance (d->test.u.acceptance, indent, is_final); - else if (d->test.kind == test::SET_OP) + else if (d->test.kind == rtx_test::SET_OP) { printf_indent (indent, "operands[%d] = ", d->test.u.opno); print_test_rtx (os, d->test); @@ -4843,8 +4846,8 @@ print_decision (output_state *os, decisi { d = trans->to->singleton (); if (!d - || d->test.kind == test::ACCEPT - || d->test.kind == test::SET_OP + || d->test.kind == rtx_test::ACCEPT + || d->test.kind == rtx_test::SET_OP || !d->if_statement_p (&label) || !test_position_available_p (os, d->test)) break; @@ -4869,7 +4872,7 @@ print_decision (output_state *os, decisi return print_state (os, to, indent, is_final); } else if (to->singleton () - && to->first->test.kind == test::ACCEPT + && to->first->test.kind == rtx_test::ACCEPT && single_statement_p (to->first->test.u.acceptance)) { /* The target of the transition is a simple "return" statement. ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-05-01 12:42 ` Richard Sandiford @ 2015-05-01 13:57 ` Jeff Law 0 siblings, 0 replies; 30+ messages in thread From: Jeff Law @ 2015-05-01 13:57 UTC (permalink / raw) To: Richard Biener, Andreas Schwab, gcc-patches, richard.sandiford On 05/01/2015 06:41 AM, Richard Sandiford wrote: > Richard Sandiford <richard.sandiford@arm.com> writes: >> Richard Biener <richard.guenther@gmail.com> writes: >>> On Thu, Apr 30, 2015 at 2:08 PM, Andreas Schwab <schwab@linux-m68k.org> wrote: >>>> Richard Sandiford <richard.sandiford@arm.com> writes: >>>> >>>>> Andreas Schwab <schwab@linux-m68k.org> writes: >>>>>> Richard Sandiford <richard.sandiford@arm.com> writes: >>>>>> >>>>>>> /* Represents a test and the action that should be taken on the result. >>>>>>> If a transition exists for the test outcome, the machine switches >>>>>>> to the transition's target state. If no suitable transition exists, >>>>>>> the machine either falls through to the next decision or, if there are no >>>>>>> more decisions to try, fails the match. */ >>>>>>> struct decision : list_head <transition> >>>>>>> { >>>>>>> decision (const test &); >>>>>>> >>>>>>> void set_parent (list_head <decision> *s); >>>>>>> bool if_statement_p (uint64_t * = 0) const; >>>>>>> >>>>>>> /* The state to which this decision belongs. */ >>>>>>> state *s; >>>>>>> >>>>>>> /* Links to other decisions in the same state. */ >>>>>>> decision *prev, *next; >>>>>>> >>>>>>> /* The test to perform. */ >>>>>>> struct test test; >>>>>>> }; >>>>>> >>>>>> ../../gcc/genrecog.c:1467: error: declaration of 'test decision::test' >>>>>> ../../gcc/genrecog.c:1051: error: changes meaning of 'test' from >>>>>> struct test' >>>>>> >>>>>> Bootstrap compiler is gcc 4.3.4. >>>>> >>>>> Bah. Does it like "::test test" instead of "struct test test"? >>>> >>>> Same error. >>> >>> You have to use a different name I believe (or -fpermissive). >> >> Hmm, but then why does it work with more recent compilers? > > Whatever the reason, I suppose the path of least resistance is to > rename the thing. > > Tested on x86_64-linux-gnu. OK to install? > > Thanks, > Richard > > > gcc/ > * genrecog.c (test): Rename to rtx_test. Update rest of file > accordingly. OK. jeff ^ permalink raw reply [flat|nested] 30+ messages in thread
* [nvptx] Re: Mostly rewrite genrecog 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford ` (3 preceding siblings ...) 2015-04-30 7:17 ` Andreas Schwab @ 2015-05-07 8:59 ` Thomas Schwinge 2015-05-07 9:14 ` Jakub Jelinek 2015-05-08 9:10 ` genrecog: Address -Wsign-compare diagnostics (was: Mostly rewrite genrecog) Thomas Schwinge 2015-05-16 8:13 ` Mostly rewrite genrecog Andreas Krebbel 6 siblings, 1 reply; 30+ messages in thread From: Thomas Schwinge @ 2015-05-07 8:59 UTC (permalink / raw) To: Richard Sandiford, Bernd Schmidt; +Cc: gcc-patches [-- Attachment #1: Type: text/plain, Size: 2809 bytes --] Hi! On Mon, 27 Apr 2015 11:20:30 +0100, Richard Sandiford <richard.sandiford@arm.com> wrote: > I think it's been the case for a while that parallel builds of GCC tend > to serialise around the compilation of insn-recog.c, especially with > higher --enable-checking settings. This patch tries to speed that > up by replacing most of genrecog with a new algorithm. [...] > Here's a comparison of the number of lines of code in insn-recog.c > before and after the patch on one target per config/ CPU: > > aarch64-linux-gnueabi 115526 38169 : 33.04% > alpha-linux-gnu 24479 10740 : 43.87% > arm-linux-gnueabi 169208 67759 : 40.04% > avr-rtems 55647 22127 : 39.76% > bfin-elf 13928 6498 : 46.65% > c6x-elf 29928 13324 : 44.52% > [...] Nice work! For a nvptx-none build of r222446 and r222860, respectively: $ wc -l {prev/,}build-gcc/gcc/insn-recog.c | head -n -1 5042 prev/build-gcc/gcc/insn-recog.c 2570 build-gcc/gcc/insn-recog.c I'm mostly illiterate regarding GCC's machine description files, and genrecog, but noticed one thing: with the genrecog rewrite, I get the following diff compared to a previous build log: build/genrecog [...]/source-gcc/gcc/common.md [...]/source-gcc/gcc/config/nvptx/nvptx.md \ insn-conditions.md > tmp-recog.c -[...]/source-gcc/gcc/config/nvptx/nvptx.md:1206: warning: operand 0 missing mode? -[...]/source-gcc/gcc/config/nvptx/nvptx.md:1206: warning: operand 1 missing mode? +Statistics for recog: + Number of decisions: 799 + longest path: 28 (code: 208) + longest backtrack: 2 (code: 136) +Statistics for split_insns: + Number of decisions: 0 + longest path: 0 (code: -1) + longest backtrack: 0 (code: -1) +Statistics for peephole2_insns: + Number of decisions: 0 + longest path: 0 (code: -1) + longest backtrack: 0 (code: -1) +Shared 655 out of 1350 states by creating 153 new states, saving 502 gcc/config/nvptx/nvptx.md: 1206 (define_insn "allocate_stack" 1207 [(set (match_operand 0 "nvptx_register_operand" "=R") 1208 (unspec [(match_operand 1 "nvptx_register_operand" "R")] 1209 UNSPEC_ALLOCA))] 1210 "" 1211 "%.\\tcall (%0), %%alloca, (%1);") Are these two (former) warnings a) something that should still be reported by genrecog, and b) something that should be addressed (Bernd)? Grüße, Thomas [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 472 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [nvptx] Re: Mostly rewrite genrecog 2015-05-07 8:59 ` [nvptx] " Thomas Schwinge @ 2015-05-07 9:14 ` Jakub Jelinek 2015-05-21 8:09 ` Thomas Schwinge 0 siblings, 1 reply; 30+ messages in thread From: Jakub Jelinek @ 2015-05-07 9:14 UTC (permalink / raw) To: Thomas Schwinge; +Cc: Richard Sandiford, Bernd Schmidt, gcc-patches On Thu, May 07, 2015 at 10:59:01AM +0200, Thomas Schwinge wrote: > build/genrecog [...]/source-gcc/gcc/common.md [...]/source-gcc/gcc/config/nvptx/nvptx.md \ > insn-conditions.md > tmp-recog.c > -[...]/source-gcc/gcc/config/nvptx/nvptx.md:1206: warning: operand 0 missing mode? > -[...]/source-gcc/gcc/config/nvptx/nvptx.md:1206: warning: operand 1 missing mode? > > gcc/config/nvptx/nvptx.md: > > 1206 (define_insn "allocate_stack" > 1207 [(set (match_operand 0 "nvptx_register_operand" "=R") > 1208 (unspec [(match_operand 1 "nvptx_register_operand" "R")] > 1209 UNSPEC_ALLOCA))] > 1210 "" > 1211 "%.\\tcall (%0), %%alloca, (%1);") > > Are these two (former) warnings a) something that should still be > reported by genrecog, Yes. > and b) something that should be addressed (Bernd)? Yes. Supposedly you want :P on both match_operand and unspec too, but as this serves not just as an insn pattern, but also as expander that needs to have this particular name, supposedly you want: (define_expand "allocate_stack" [(match_operand 0 "nvptx_register_operand") (match_operand 1 "nvptx_register_operand")] "" { if (TARGET_ABI64) emit_insn (gen_allocate_stack_di (operands[0], operands[1])); else emit_insn (gen_allocate_stack_si (operands[0], operands[1])); DONE; }) (define_insn "allocate_stack_<mode>" [(set (match_operand:P 0 "nvptx_register_operand" "=R") (unspec:P [(match_operand:P 1 "nvptx_register_operand" "R")] UNSPEC_ALLOCA))] "" "%.\\tcall (%0), %%alloca, (%1);") rr so. Of course, as even latest Cuda drop doesn't support alloca, this is quite dubious, perhaps better would be sorry on it. BTW, with Cuda 7.0, even printf doesn't work anymore, is that known? Jakub ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [nvptx] Re: Mostly rewrite genrecog 2015-05-07 9:14 ` Jakub Jelinek @ 2015-05-21 8:09 ` Thomas Schwinge 2015-05-21 11:57 ` Bernd Schmidt 0 siblings, 1 reply; 30+ messages in thread From: Thomas Schwinge @ 2015-05-21 8:09 UTC (permalink / raw) To: Jakub Jelinek, Bernd Schmidt, gcc-patches; +Cc: Richard Sandiford [-- Attachment #1: Type: text/plain, Size: 3817 bytes --] Hi! On Thu, 7 May 2015 11:14:37 +0200, Jakub Jelinek <jakub@redhat.com> wrote: > On Thu, May 07, 2015 at 10:59:01AM +0200, Thomas Schwinge wrote: > > build/genrecog [...]/source-gcc/gcc/common.md [...]/source-gcc/gcc/config/nvptx/nvptx.md \ > > insn-conditions.md > tmp-recog.c > > -[...]/source-gcc/gcc/config/nvptx/nvptx.md:1206: warning: operand 0 missing mode? > > -[...]/source-gcc/gcc/config/nvptx/nvptx.md:1206: warning: operand 1 missing mode? > > > > gcc/config/nvptx/nvptx.md: > > > > 1206 (define_insn "allocate_stack" > > 1207 [(set (match_operand 0 "nvptx_register_operand" "=R") > > 1208 (unspec [(match_operand 1 "nvptx_register_operand" "R")] > > 1209 UNSPEC_ALLOCA))] > > 1210 "" > > 1211 "%.\\tcall (%0), %%alloca, (%1);") > > > > Are these two (former) warnings a) something that should still be > > reported by genrecog, > > Yes. <http://news.gmane.org/find-root.php?message_id=%3C87twvjtrf4.fsf%40e105548-lin.cambridge.arm.com%3E>. > > and b) something that should be addressed (Bernd)? > > Yes. Supposedly you want :P on both match_operand and unspec too, but > as this serves not just as an insn pattern, but also as expander that > needs to have this particular name, supposedly you want: > > (define_expand "allocate_stack" > [(match_operand 0 "nvptx_register_operand") > (match_operand 1 "nvptx_register_operand")] > "" > { > if (TARGET_ABI64) > emit_insn (gen_allocate_stack_di (operands[0], operands[1])); > else > emit_insn (gen_allocate_stack_si (operands[0], operands[1])); > DONE; > }) > > (define_insn "allocate_stack_<mode>" > [(set (match_operand:P 0 "nvptx_register_operand" "=R") > (unspec:P [(match_operand:P 1 "nvptx_register_operand" "R")] > UNSPEC_ALLOCA))] > "" > "%.\\tcall (%0), %%alloca, (%1);") > > rr so. OK to commit? commit 004e521e8dd1c0236a55e9a69a17cc3333c2a41d Author: Thomas Schwinge <thomas@codesourcery.com> Date: Thu May 7 11:30:26 2015 +0200 [nvptx] Address genrecog warnings 2015-05-21 Jakub Jelinek <jakub@redhat.com> gcc/ * config/nvptx/nvptx.md (allocate_stack): Rename to... (allocate_stack_<mode>): ... this, and add :P on both match_operand and unspec. (allocate_stack): New expander. --- gcc/config/nvptx/nvptx.md | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git gcc/config/nvptx/nvptx.md gcc/config/nvptx/nvptx.md index c30de36..a49786c 100644 --- gcc/config/nvptx/nvptx.md +++ gcc/config/nvptx/nvptx.md @@ -1203,10 +1203,22 @@ sorry ("target cannot support nonlocal goto."); }) -(define_insn "allocate_stack" - [(set (match_operand 0 "nvptx_register_operand" "=R") - (unspec [(match_operand 1 "nvptx_register_operand" "R")] - UNSPEC_ALLOCA))] +(define_expand "allocate_stack" + [(match_operand 0 "nvptx_register_operand") + (match_operand 1 "nvptx_register_operand")] + "" +{ + if (TARGET_ABI64) + emit_insn (gen_allocate_stack_di (operands[0], operands[1])); + else + emit_insn (gen_allocate_stack_si (operands[0], operands[1])); + DONE; +}) + +(define_insn "allocate_stack_<mode>" + [(set (match_operand:P 0 "nvptx_register_operand" "=R") + (unspec:P [(match_operand:P 1 "nvptx_register_operand" "R")] + UNSPEC_ALLOCA))] "" "%.\\tcall (%0), %%alloca, (%1);") > Of course, as even latest Cuda drop doesn't support alloca, this is > quite dubious, perhaps better would be sorry on it. > > BTW, with Cuda 7.0, even printf doesn't work anymore, is that known? I have not yet used that version of CUDA, so don't know about this. :-| Grüße, Thomas [-- Attachment #2: Type: application/pgp-signature, Size: 472 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [nvptx] Re: Mostly rewrite genrecog 2015-05-21 8:09 ` Thomas Schwinge @ 2015-05-21 11:57 ` Bernd Schmidt 0 siblings, 0 replies; 30+ messages in thread From: Bernd Schmidt @ 2015-05-21 11:57 UTC (permalink / raw) To: Thomas Schwinge, Jakub Jelinek, gcc-patches; +Cc: Richard Sandiford On 05/21/2015 09:12 AM, Thomas Schwinge wrote: > > OK to commit? > > gcc/ > * config/nvptx/nvptx.md (allocate_stack): Rename to... > (allocate_stack_<mode>): ... this, and add :P on both > match_operand and unspec. > (allocate_stack): New expander. If you really want to. It doesn't work yet in ptxas so it's a little pointless to spend effort on it. Bernd ^ permalink raw reply [flat|nested] 30+ messages in thread
* genrecog: Address -Wsign-compare diagnostics (was: Mostly rewrite genrecog) 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford ` (4 preceding siblings ...) 2015-05-07 8:59 ` [nvptx] " Thomas Schwinge @ 2015-05-08 9:10 ` Thomas Schwinge 2015-05-08 14:43 ` genrecog: Address -Wsign-compare diagnostics Richard Sandiford 2015-05-08 18:39 ` Jeff Law 2015-05-16 8:13 ` Mostly rewrite genrecog Andreas Krebbel 6 siblings, 2 replies; 30+ messages in thread From: Thomas Schwinge @ 2015-05-08 9:10 UTC (permalink / raw) To: Richard Sandiford, gcc-patches [-- Attachment #1: Type: text/plain, Size: 3572 bytes --] Hi! On Mon, 27 Apr 2015 11:20:30 +0100, Richard Sandiford <richard.sandiford@arm.com> wrote: > This patch [...] by replacing most of genrecog [...] OK to commit? Is it a bug that I'm seeing these warnings only in the stage 1 build with the bootstrap GCC 4.6 compiler, but not anymore later on? (I have not verified the C++ standard on the rules for »comparison between signed and unsigned integer expressions«.) commit efef4f38205a13da90ca19b6eec1a6526756b433 Author: Thomas Schwinge <thomas@codesourcery.com> Date: Fri May 8 10:55:19 2015 +0200 genrecog: Address -Wsign-compare diagnostics. g++-4.6 [...] [...]/gcc/genrecog.c [...]/gcc/genrecog.c: In function 'state_size find_subroutines(routine_type, state*, vec<state*>&)': [...]/gcc/genrecog.c:3338:35: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] [...]/gcc/genrecog.c:3347:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] [...]/gcc/genrecog.c:3359:29: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] [...]/gcc/genrecog.c:3365:32: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] 3305 state_size size; [...] 3337 state_size to_size = find_subroutines (type, trans->to, procs); 3338 if (d->next && to_size.depth > MAX_DEPTH) [...] 3347 if (to_size.num_statements < MIN_NUM_STATEMENTS) [...] 3359 if (size.num_statements > MAX_NUM_STATEMENTS) [...] 3365 && size.num_statements > MAX_NUM_STATEMENTS) 175 static const int MAX_DEPTH = 6; [...] 179 static const int MIN_NUM_STATEMENTS = 5; [...] 185 static const int MAX_NUM_STATEMENTS = 200; [...] 3258 struct state_size 3259 { [...] 3261 unsigned int num_statements; [...] 3265 unsigned int depth; 3266 }; gcc/ * genrecog.c (MAX_DEPTH, MIN_NUM_STATEMENTS, MAX_NUM_STATEMENTS): Change to unsigned int. --- gcc/genrecog.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git gcc/genrecog.c gcc/genrecog.c index 73e7995..653f753 100644 --- gcc/genrecog.c +++ gcc/genrecog.c @@ -172,17 +172,17 @@ static const bool force_unique_params_p = true; /* The maximum (approximate) depth of block nesting that an individual routine or subroutine should have. This limit is about keeping the output readable rather than reducing compile time. */ -static const int MAX_DEPTH = 6; +static const unsigned int MAX_DEPTH = 6; /* The minimum number of pseudo-statements that a state must have before we split it out into a subroutine. */ -static const int MIN_NUM_STATEMENTS = 5; +static const unsigned int MIN_NUM_STATEMENTS = 5; /* The number of pseudo-statements a state can have before we consider splitting out substates into subroutines. This limit is about avoiding compile-time problems with very big functions (and also about keeping functions within --param optimization limits, etc.). */ -static const int MAX_NUM_STATEMENTS = 200; +static const unsigned int MAX_NUM_STATEMENTS = 200; /* The minimum number of pseudo-statements that can be used in a pattern routine. */ Grüße, Thomas [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 472 bytes --] ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: genrecog: Address -Wsign-compare diagnostics 2015-05-08 9:10 ` genrecog: Address -Wsign-compare diagnostics (was: Mostly rewrite genrecog) Thomas Schwinge @ 2015-05-08 14:43 ` Richard Sandiford 2015-05-08 18:39 ` Jeff Law 1 sibling, 0 replies; 30+ messages in thread From: Richard Sandiford @ 2015-05-08 14:43 UTC (permalink / raw) To: Thomas Schwinge; +Cc: gcc-patches Thomas Schwinge <thomas@codesourcery.com> writes: > Hi! > > On Mon, 27 Apr 2015 11:20:30 +0100, Richard Sandiford > <richard.sandiford@arm.com> wrote: >> This patch [...] by replacing most of genrecog [...] > > OK to commit? Looks good to me FWIW. Probably counts as obvious. Thanks, Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: genrecog: Address -Wsign-compare diagnostics 2015-05-08 9:10 ` genrecog: Address -Wsign-compare diagnostics (was: Mostly rewrite genrecog) Thomas Schwinge 2015-05-08 14:43 ` genrecog: Address -Wsign-compare diagnostics Richard Sandiford @ 2015-05-08 18:39 ` Jeff Law 1 sibling, 0 replies; 30+ messages in thread From: Jeff Law @ 2015-05-08 18:39 UTC (permalink / raw) To: Thomas Schwinge, Richard Sandiford, gcc-patches On 05/08/2015 03:09 AM, Thomas Schwinge wrote: > Hi! > > On Mon, 27 Apr 2015 11:20:30 +0100, Richard Sandiford <richard.sandiford@arm.com> wrote: >> This patch [...] by replacing most of genrecog [...] > > OK to commit? > > Is it a bug that I'm seeing these warnings only in the stage 1 build with > the bootstrap GCC 4.6 compiler, but not anymore later on? (I have not > verified the C++ standard on the rules for »comparison between signed and > unsigned integer expressions«.) > > commit efef4f38205a13da90ca19b6eec1a6526756b433 > Author: Thomas Schwinge <thomas@codesourcery.com> > Date: Fri May 8 10:55:19 2015 +0200 > > genrecog: Address -Wsign-compare diagnostics. > > g++-4.6 [...] [...]/gcc/genrecog.c > [...]/gcc/genrecog.c: In function 'state_size find_subroutines(routine_type, state*, vec<state*>&)': > [...]/gcc/genrecog.c:3338:35: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] > [...]/gcc/genrecog.c:3347:37: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] > [...]/gcc/genrecog.c:3359:29: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] > [...]/gcc/genrecog.c:3365:32: warning: comparison between signed and unsigned integer expressions [-Wsign-compare] > > 3305 state_size size; > [...] > 3337 state_size to_size = find_subroutines (type, trans->to, procs); > 3338 if (d->next && to_size.depth > MAX_DEPTH) > [...] > 3347 if (to_size.num_statements < MIN_NUM_STATEMENTS) > [...] > 3359 if (size.num_statements > MAX_NUM_STATEMENTS) > [...] > 3365 && size.num_statements > MAX_NUM_STATEMENTS) > > 175 static const int MAX_DEPTH = 6; > [...] > 179 static const int MIN_NUM_STATEMENTS = 5; > [...] > 185 static const int MAX_NUM_STATEMENTS = 200; > [...] > 3258 struct state_size > 3259 { > [...] > 3261 unsigned int num_statements; > [...] > 3265 unsigned int depth; > 3266 }; > > gcc/ > * genrecog.c (MAX_DEPTH, MIN_NUM_STATEMENTS, MAX_NUM_STATEMENTS): > Change to unsigned int. > --- > gcc/genrecog.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) OK with a ChangeLog and the usual testing. Or you can verify the output of genrecog doesn't change before/after if you don't want to go through the full bootstrap. jeff ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford ` (5 preceding siblings ...) 2015-05-08 9:10 ` genrecog: Address -Wsign-compare diagnostics (was: Mostly rewrite genrecog) Thomas Schwinge @ 2015-05-16 8:13 ` Andreas Krebbel 2015-05-17 22:05 ` Richard Sandiford 6 siblings, 1 reply; 30+ messages in thread From: Andreas Krebbel @ 2015-05-16 8:13 UTC (permalink / raw) To: gcc-patches, richard.sandiford Hi Richard, I see regressions with the current IBM z13 vector patchset which appear to be related to the new genrecog. The following two insn definitions only differ in the mode and predicate of the shift count operand. (define_insn "lshr<mode>3" [(set (match_operand:VI 0 "register_operand" "=v") (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") (match_operand:SI 2 "shift_count_or_setmem_operand" "Y")))] "TARGET_VX" "vesrl<bhfgq>\t%v0,%v1,%Y2" [(set_attr "op_type" "VRS")]) (define_insn "vlshr<mode>3" [(set (match_operand:VI 0 "register_operand" "=v") (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") (match_operand:VI 2 "register_operand" "v")))] "TARGET_VX" "vesrlv<bhfgq>\t%v0,%v1,%v2" [(set_attr "op_type" "VRR")]) However, the insn-recog.c code only seem to check the predicate. This is a problem since shift_count_or_setmem_operand does not check the mode. if (shift_count_or_setmem_operand (operands[2], SImode) && #line 717 "/home3/andreas/patched/../gcc/gcc/config/s390/vector.md" (TARGET_VX)) return 600; /* lshrv2qi3 */ if (register_operand (operands[2], V2QImode) && #line 747 "/home3/andreas/patched/../gcc/gcc/config/s390/vector.md" (TARGET_VX)) return 630; /* vlshrv2qi3 */ break; I could add a mode check to the predicate. However, I just wanted to check whether this change was intentional. Bye, -Andreas- ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-05-16 8:13 ` Mostly rewrite genrecog Andreas Krebbel @ 2015-05-17 22:05 ` Richard Sandiford 2015-05-22 16:14 ` Andreas Krebbel 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-05-17 22:05 UTC (permalink / raw) To: Andreas Krebbel; +Cc: gcc-patches Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes: > Hi Richard, > > I see regressions with the current IBM z13 vector patchset which appear to be related to the new > genrecog. > > The following two insn definitions only differ in the mode and predicate of the shift count operand. > > (define_insn "lshr<mode>3" > [(set (match_operand:VI 0 "register_operand" "=v") > (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") > (match_operand:SI 2 "shift_count_or_setmem_operand" "Y")))] > "TARGET_VX" > "vesrl<bhfgq>\t%v0,%v1,%Y2" > [(set_attr "op_type" "VRS")]) > > (define_insn "vlshr<mode>3" > [(set (match_operand:VI 0 "register_operand" "=v") > (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") > (match_operand:VI 2 "register_operand" "v")))] > "TARGET_VX" > "vesrlv<bhfgq>\t%v0,%v1,%v2" > [(set_attr "op_type" "VRR")]) > > > However, the insn-recog.c code only seem to check the predicate. This is a problem since > shift_count_or_setmem_operand does not check the mode. Yeah, it's a bug if a "non-special" predicate doesn't check the mode. Even old genreog relied on that: /* After factoring, try to simplify the tests on any one node. Tests that are useful for switch statements are recognizable by having only a single test on a node -- we'll be manipulating nodes with multiple tests: If we have mode tests or code tests that are redundant with predicates, remove them. */ although it sounds like the old optimisation didn't trigger for your case. genpreds.c:mark_mode_tests is supposed to add these tests automatically if needed. I suppose it isn't doing so here because the predicate accepts const_int and because of: /* Given an RTL expression EXP, find all subexpressions which we may assume to perform mode tests. Normal MATCH_OPERAND does; MATCH_CODE does if it applies to the whole expression and accepts CONST_INT or CONST_DOUBLE; and we have to assume that MATCH_TEST does not. [...] */ static void mark_mode_tests (rtx exp) { switch (GET_CODE (exp)) { [...] case MATCH_CODE: if (XSTR (exp, 1)[0] != '\0' || (!strstr (XSTR (exp, 0), "const_int") && !strstr (XSTR (exp, 0), "const_double"))) NO_MODE_TEST (exp) = 1; break; The code matches the comment, but it doesn't look right. Perhaps it was supposed to mean match_codes that _only_ contain const_int and const_double? Knowing that the rtx is one of those codes guarantees that the mode is VOIDmode, but a match_code that includes other rtxes as well doesn't itself test the mode of those rtxes. Even then, a predicate that matches const_ints and is passed SImode mustn't accept const_ints outside the SImode range. I suppose we just rely on all predicates to perform some kind of range check. (The standard ones do of course.) As a quick workaround, try replacing the case above with: case MATCH_CODE: if (XSTR (exp, 1)[0] != '\0') NO_MODE_TEST (exp) = 1; break; I'll try to come up with a better fix in the meantime. Thanks, Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-05-17 22:05 ` Richard Sandiford @ 2015-05-22 16:14 ` Andreas Krebbel 2015-05-22 16:32 ` Richard Sandiford 0 siblings, 1 reply; 30+ messages in thread From: Andreas Krebbel @ 2015-05-22 16:14 UTC (permalink / raw) To: gcc-patches, richard.sandiford On 05/17/2015 11:12 PM, Richard Sandiford wrote: > Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes: >> Hi Richard, >> >> I see regressions with the current IBM z13 vector patchset which appear to be related to the new >> genrecog. >> >> The following two insn definitions only differ in the mode and predicate of the shift count operand. >> >> (define_insn "lshr<mode>3" >> [(set (match_operand:VI 0 "register_operand" "=v") >> (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") >> (match_operand:SI 2 "shift_count_or_setmem_operand" "Y")))] >> "TARGET_VX" >> "vesrl<bhfgq>\t%v0,%v1,%Y2" >> [(set_attr "op_type" "VRS")]) >> >> (define_insn "vlshr<mode>3" >> [(set (match_operand:VI 0 "register_operand" "=v") >> (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") >> (match_operand:VI 2 "register_operand" "v")))] >> "TARGET_VX" >> "vesrlv<bhfgq>\t%v0,%v1,%v2" >> [(set_attr "op_type" "VRR")]) >> >> >> However, the insn-recog.c code only seem to check the predicate. This is a problem since >> shift_count_or_setmem_operand does not check the mode. > > Yeah, it's a bug if a "non-special" predicate doesn't check the mode. > Even old genreog relied on that: > > /* After factoring, try to simplify the tests on any one node. > Tests that are useful for switch statements are recognizable > by having only a single test on a node -- we'll be manipulating > nodes with multiple tests: > > If we have mode tests or code tests that are redundant with > predicates, remove them. */ > > although it sounds like the old optimisation didn't trigger for your case. > > genpreds.c:mark_mode_tests is supposed to add these tests automatically > if needed. I suppose it isn't doing so here because the predicate > accepts const_int and because of: > > /* Given an RTL expression EXP, find all subexpressions which we may > assume to perform mode tests. Normal MATCH_OPERAND does; > MATCH_CODE does if it applies to the whole expression and accepts > CONST_INT or CONST_DOUBLE; and we have to assume that MATCH_TEST > does not. [...] > */ > static void > mark_mode_tests (rtx exp) > { > switch (GET_CODE (exp)) > { > [...] > case MATCH_CODE: > if (XSTR (exp, 1)[0] != '\0' > || (!strstr (XSTR (exp, 0), "const_int") > && !strstr (XSTR (exp, 0), "const_double"))) > NO_MODE_TEST (exp) = 1; > break; > > The code matches the comment, but it doesn't look right. Perhaps it > was supposed to mean match_codes that _only_ contain const_int and > const_double? Knowing that the rtx is one of those codes guarantees > that the mode is VOIDmode, but a match_code that includes other rtxes > as well doesn't itself test the mode of those rtxes. > > Even then, a predicate that matches const_ints and is passed SImode > mustn't accept const_ints outside the SImode range. I suppose we > just rely on all predicates to perform some kind of range check. > (The standard ones do of course.) > > As a quick workaround, try replacing the case above with: > > case MATCH_CODE: > if (XSTR (exp, 1)[0] != '\0') > NO_MODE_TEST (exp) = 1; > break; > > I'll try to come up with a better fix in the meantime. Thanks for looking into this! I've applied a patch adding a mode check to shift_count_or_setmem_operand which as expected fixes the issue for me. -Andreas- ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: Mostly rewrite genrecog 2015-05-22 16:14 ` Andreas Krebbel @ 2015-05-22 16:32 ` Richard Sandiford 2015-05-29 17:39 ` RFA: Fix mode checks for possibly-constant predicates Richard Sandiford 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-05-22 16:32 UTC (permalink / raw) To: Andreas Krebbel; +Cc: gcc-patches Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes: > On 05/17/2015 11:12 PM, Richard Sandiford wrote: >> Andreas Krebbel <krebbel@linux.vnet.ibm.com> writes: >>> Hi Richard, >>> >>> I see regressions with the current IBM z13 vector patchset which appear to be related to the new >>> genrecog. >>> >>> The following two insn definitions only differ in the mode and predicate of the shift count operand. >>> >>> (define_insn "lshr<mode>3" >>> [(set (match_operand:VI 0 "register_operand" "=v") >>> (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") >>> (match_operand:SI 2 "shift_count_or_setmem_operand" "Y")))] >>> "TARGET_VX" >>> "vesrl<bhfgq>\t%v0,%v1,%Y2" >>> [(set_attr "op_type" "VRS")]) >>> >>> (define_insn "vlshr<mode>3" >>> [(set (match_operand:VI 0 "register_operand" "=v") >>> (lshiftrt:VI (match_operand:VI 1 "register_operand" "v") >>> (match_operand:VI 2 "register_operand" "v")))] >>> "TARGET_VX" >>> "vesrlv<bhfgq>\t%v0,%v1,%v2" >>> [(set_attr "op_type" "VRR")]) >>> >>> >>> However, the insn-recog.c code only seem to check the predicate. This is a problem since >>> shift_count_or_setmem_operand does not check the mode. >> >> Yeah, it's a bug if a "non-special" predicate doesn't check the mode. >> Even old genreog relied on that: >> >> /* After factoring, try to simplify the tests on any one node. >> Tests that are useful for switch statements are recognizable >> by having only a single test on a node -- we'll be manipulating >> nodes with multiple tests: >> >> If we have mode tests or code tests that are redundant with >> predicates, remove them. */ >> >> although it sounds like the old optimisation didn't trigger for your case. >> >> genpreds.c:mark_mode_tests is supposed to add these tests automatically >> if needed. I suppose it isn't doing so here because the predicate >> accepts const_int and because of: >> >> /* Given an RTL expression EXP, find all subexpressions which we may >> assume to perform mode tests. Normal MATCH_OPERAND does; >> MATCH_CODE does if it applies to the whole expression and accepts >> CONST_INT or CONST_DOUBLE; and we have to assume that MATCH_TEST >> does not. [...] >> */ >> static void >> mark_mode_tests (rtx exp) >> { >> switch (GET_CODE (exp)) >> { >> [...] >> case MATCH_CODE: >> if (XSTR (exp, 1)[0] != '\0' >> || (!strstr (XSTR (exp, 0), "const_int") >> && !strstr (XSTR (exp, 0), "const_double"))) >> NO_MODE_TEST (exp) = 1; >> break; >> >> The code matches the comment, but it doesn't look right. Perhaps it >> was supposed to mean match_codes that _only_ contain const_int and >> const_double? Knowing that the rtx is one of those codes guarantees >> that the mode is VOIDmode, but a match_code that includes other rtxes >> as well doesn't itself test the mode of those rtxes. >> >> Even then, a predicate that matches const_ints and is passed SImode >> mustn't accept const_ints outside the SImode range. I suppose we >> just rely on all predicates to perform some kind of range check. >> (The standard ones do of course.) >> >> As a quick workaround, try replacing the case above with: >> >> case MATCH_CODE: >> if (XSTR (exp, 1)[0] != '\0') >> NO_MODE_TEST (exp) = 1; >> break; >> >> I'll try to come up with a better fix in the meantime. > > Thanks for looking into this! > I've applied a patch adding a mode check to > shift_count_or_setmem_operand which as expected fixes > the issue for me. OK. After a false start earlier in the week, I've now got a patch that I think fixes genpreds.c "properly". Hope to post it soon. Thanks, Richard ^ permalink raw reply [flat|nested] 30+ messages in thread
* RFA: Fix mode checks for possibly-constant predicates 2015-05-22 16:32 ` Richard Sandiford @ 2015-05-29 17:39 ` Richard Sandiford 2015-05-29 19:27 ` Richard Henderson 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-05-29 17:39 UTC (permalink / raw) To: gcc-patches; +Cc: Andreas Krebbel genrecog relies on a predicate foo_operand (op, mode) checking that OP really does have mode MODE, with VOIDmode acting as a wildcard. This was true even with the old genrecog, but as Andreas found on s390x, new genrecog is being a bit more aggressive about it. The problem is that at the moment, genpreds drops the mode check for anything that can accept a CONST_INT or CONST_DOUBLE, on the basis that constant integers have a recorded mode of VOIDmode whatever their "real" logical mode is. But that means that a predicate that accepts constant integers and other rtxes like REG won't check the modes of those other rtxes either. Even dropping the check for CONST_DOUBLE is wrong, since it can be a floating-point constant with a real mode. This patch therefore adjusts the automatic mode test to allow GET_MODE (op) == VOIDmode in cases where that's necessary. It also includes CONST_WIDE_INT in the list of constant integer codes. Bootstrapped & regression-tested on x86_64-linux-gnu. OK to install? Thanks, Richard gcc/ * genpreds.c (mark_mode_tests): Mark all MATCH_CODEs as NO_MODE_TEST. (add_mode_tests): Don't add mode tests if the predicate only accepts scalar constant integers. Otherwise, allow the mode of "op" to be VOIDmode if the predicate does accept such integers. Index: gcc/genpreds.c =================================================================== --- gcc/genpreds.c 2015-05-22 16:49:55.454319576 +0100 +++ gcc/genpreds.c 2015-05-23 10:46:58.187370110 +0100 @@ -218,11 +218,11 @@ needs_variable (rtx exp, const char *var /* Given an RTL expression EXP, find all subexpressions which we may assume to perform mode tests. Normal MATCH_OPERAND does; - MATCH_CODE does if it applies to the whole expression and accepts - CONST_INT or CONST_DOUBLE; and we have to assume that MATCH_TEST - does not. These combine in almost-boolean fashion - the only - exception is that (not X) must be assumed not to perform a mode - test, whether or not X does. + MATCH_CODE doesn't as such (although certain codes always have + VOIDmode); and we have to assume that MATCH_TEST does not. + These combine in almost-boolean fashion - the only exception is + that (not X) must be assumed not to perform a mode test, whether + or not X does. The mark is the RTL /v flag, which is true for subexpressions which do *not* perform mode tests. @@ -244,10 +244,7 @@ mark_mode_tests (rtx exp) break; case MATCH_CODE: - if (XSTR (exp, 1)[0] != '\0' - || (!strstr (XSTR (exp, 0), "const_int") - && !strstr (XSTR (exp, 0), "const_double"))) - NO_MODE_TEST (exp) = 1; + NO_MODE_TEST (exp) = 1; break; case MATCH_TEST: @@ -313,6 +310,33 @@ add_mode_tests (struct pred_data *p) if (p->special) return; + /* Check whether the predicate accepts const scalar ints (which always + have a stored mode of VOIDmode, but logically have a real mode) + and whether it matches anything besides const scalar ints. */ + bool matches_const_scalar_int_p = false; + bool matches_other_p = false; + for (int i = 0; i < NUM_RTX_CODE; ++i) + if (p->codes[i]) + switch (i) + { + CASE_CONST_SCALAR_INT: + matches_const_scalar_int_p = true; + break; + + default: + matches_other_p = true; + break; + } + + /* There's no need for a mode check if the predicate only accepts + constant integers. The code checks in the predicate are enough + to establish that the mode is VOIDmode. + + Note that the predicate itself should check whether a scalar + integer is in range of the given mode. */ + if (!matches_other_p && !p->codes[CONST_DOUBLE]) + return; + mark_mode_tests (p->exp); /* If the whole expression already tests the mode, we're done. */ @@ -320,7 +344,11 @@ add_mode_tests (struct pred_data *p) return; match_test_exp = rtx_alloc (MATCH_TEST); - XSTR (match_test_exp, 0) = "mode == VOIDmode || GET_MODE (op) == mode"; + if (matches_const_scalar_int_p) + XSTR (match_test_exp, 0) = ("mode == VOIDmode || GET_MODE (op) == mode" + " || GET_MODE (op) == VOIDmode"); + else + XSTR (match_test_exp, 0) = "mode == VOIDmode || GET_MODE (op) == mode"; and_exp = rtx_alloc (AND); XEXP (and_exp, 1) = match_test_exp; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA: Fix mode checks for possibly-constant predicates 2015-05-29 17:39 ` RFA: Fix mode checks for possibly-constant predicates Richard Sandiford @ 2015-05-29 19:27 ` Richard Henderson 2015-06-03 7:23 ` Richard Sandiford 0 siblings, 1 reply; 30+ messages in thread From: Richard Henderson @ 2015-05-29 19:27 UTC (permalink / raw) To: gcc-patches, Andreas Krebbel, richard.sandiford On 05/29/2015 10:23 AM, Richard Sandiford wrote: > + /* Check whether the predicate accepts const scalar ints (which always > + have a stored mode of VOIDmode, but logically have a real mode) > + and whether it matches anything besides const scalar ints. */ > + bool matches_const_scalar_int_p = false; > + bool matches_other_p = false; > + for (int i = 0; i < NUM_RTX_CODE; ++i) > + if (p->codes[i]) > + switch (i) > + { > + CASE_CONST_SCALAR_INT: > + matches_const_scalar_int_p = true; > + break; > + > + default: > + matches_other_p = true; > + break; > + } > + > + /* There's no need for a mode check if the predicate only accepts > + constant integers. The code checks in the predicate are enough > + to establish that the mode is VOIDmode. > + > + Note that the predicate itself should check whether a scalar > + integer is in range of the given mode. */ > + if (!matches_other_p && !p->codes[CONST_DOUBLE]) > + return; I think perhaps it would be cleaner to not use CASE_CONST_SCALAR_INT, and then do switch (i) { case CONST_INT: case CONST_WIDE_INT: matches_const_scalar_int_p = true; break; case CONST_DOUBLE: if (!TARGET_SUPPORTS_WIDE_INT) matches_const_scalar_int_p = true; matches_other_p = true; break; default: matches_other_p = true; break; } if (!matches_other_p) return; Otherwise ok. r~ ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA: Fix mode checks for possibly-constant predicates 2015-05-29 19:27 ` Richard Henderson @ 2015-06-03 7:23 ` Richard Sandiford 2015-06-03 9:39 ` Richard Sandiford 0 siblings, 1 reply; 30+ messages in thread From: Richard Sandiford @ 2015-06-03 7:23 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches, Andreas Krebbel Richard Henderson <rth@redhat.com> writes: > On 05/29/2015 10:23 AM, Richard Sandiford wrote: >> + /* Check whether the predicate accepts const scalar ints (which always >> + have a stored mode of VOIDmode, but logically have a real mode) >> + and whether it matches anything besides const scalar ints. */ >> + bool matches_const_scalar_int_p = false; >> + bool matches_other_p = false; >> + for (int i = 0; i < NUM_RTX_CODE; ++i) >> + if (p->codes[i]) >> + switch (i) >> + { >> + CASE_CONST_SCALAR_INT: >> + matches_const_scalar_int_p = true; >> + break; >> + >> + default: >> + matches_other_p = true; >> + break; >> + } >> + >> + /* There's no need for a mode check if the predicate only accepts >> + constant integers. The code checks in the predicate are enough >> + to establish that the mode is VOIDmode. >> + >> + Note that the predicate itself should check whether a scalar >> + integer is in range of the given mode. */ >> + if (!matches_other_p && !p->codes[CONST_DOUBLE]) >> + return; > > I think perhaps it would be cleaner to not use CASE_CONST_SCALAR_INT, > and then do > > switch (i) > { > case CONST_INT: > case CONST_WIDE_INT: > matches_const_scalar_int_p = true; > break; > > case CONST_DOUBLE: > if (!TARGET_SUPPORTS_WIDE_INT) > matches_const_scalar_int_p = true; > matches_other_p = true; > break; > > default: > matches_other_p = true; > break; > } > > if (!matches_other_p) > return; > > Otherwise ok. Ah, yeah, that's better. Here's what I applied after testing on x86_64-linux-gnu. Thanks, Richard gcc/ * genpreds.c (mark_mode_tests): Mark all MATCH_CODEs as NO_MODE_TEST. (add_mode_tests): Don't add mode tests if the predicate only accepts scalar constant integers. Otherwise, allow the mode of "op" to be VOIDmode if the predicate does accept such integers. Index: gcc/genpreds.c =================================================================== --- gcc/genpreds.c 2015-06-02 13:32:21.394938060 +0100 +++ gcc/genpreds.c 2015-06-02 13:34:50.377221396 +0100 @@ -218,11 +218,11 @@ needs_variable (rtx exp, const char *var /* Given an RTL expression EXP, find all subexpressions which we may assume to perform mode tests. Normal MATCH_OPERAND does; - MATCH_CODE does if it applies to the whole expression and accepts - CONST_INT or CONST_DOUBLE; and we have to assume that MATCH_TEST - does not. These combine in almost-boolean fashion - the only - exception is that (not X) must be assumed not to perform a mode - test, whether or not X does. + MATCH_CODE doesn't as such (although certain codes always have + VOIDmode); and we have to assume that MATCH_TEST does not. + These combine in almost-boolean fashion - the only exception is + that (not X) must be assumed not to perform a mode test, whether + or not X does. The mark is the RTL /v flag, which is true for subexpressions which do *not* perform mode tests. @@ -244,10 +244,7 @@ mark_mode_tests (rtx exp) break; case MATCH_CODE: - if (XSTR (exp, 1)[0] != '\0' - || (!strstr (XSTR (exp, 0), "const_int") - && !strstr (XSTR (exp, 0), "const_double"))) - NO_MODE_TEST (exp) = 1; + NO_MODE_TEST (exp) = 1; break; case MATCH_TEST: @@ -313,6 +310,40 @@ add_mode_tests (struct pred_data *p) if (p->special) return; + /* Check whether the predicate accepts const scalar ints (which always + have a stored mode of VOIDmode, but logically have a real mode) + and whether it matches anything besides const scalar ints. */ + bool matches_const_scalar_int_p = false; + bool matches_other_p = false; + for (int i = 0; i < NUM_RTX_CODE; ++i) + if (p->codes[i]) + switch (i) + { + case CONST_INT: + case CONST_WIDE_INT: + matches_const_scalar_int_p = true; + break; + + case CONST_DOUBLE: + if (!TARGET_SUPPORTS_WIDE_INT) + matches_const_scalar_int_p = true; + matches_other_p = true; + break; + + default: + matches_other_p = true; + break; + } + + /* There's no need for a mode check if the predicate only accepts + constant integers. The code checks in the predicate are enough + to establish that the mode is VOIDmode. + + Note that the predicate itself should check whether a scalar + integer is in range of the given mode. */ + if (!matches_other_p) + return; + mark_mode_tests (p->exp); /* If the whole expression already tests the mode, we're done. */ @@ -320,7 +351,11 @@ add_mode_tests (struct pred_data *p) return; match_test_exp = rtx_alloc (MATCH_TEST); - XSTR (match_test_exp, 0) = "mode == VOIDmode || GET_MODE (op) == mode"; + if (matches_const_scalar_int_p) + XSTR (match_test_exp, 0) = ("mode == VOIDmode || GET_MODE (op) == mode" + " || GET_MODE (op) == VOIDmode"); + else + XSTR (match_test_exp, 0) = "mode == VOIDmode || GET_MODE (op) == mode"; and_exp = rtx_alloc (AND); XEXP (and_exp, 1) = match_test_exp; ^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: RFA: Fix mode checks for possibly-constant predicates 2015-06-03 7:23 ` Richard Sandiford @ 2015-06-03 9:39 ` Richard Sandiford 0 siblings, 0 replies; 30+ messages in thread From: Richard Sandiford @ 2015-06-03 9:39 UTC (permalink / raw) To: Richard Henderson; +Cc: gcc-patches, Andreas Krebbel Richard Sandiford <richard.sandiford@arm.com> writes: > Richard Henderson <rth@redhat.com> writes: >> On 05/29/2015 10:23 AM, Richard Sandiford wrote: >>> + /* Check whether the predicate accepts const scalar ints (which always >>> + have a stored mode of VOIDmode, but logically have a real mode) >>> + and whether it matches anything besides const scalar ints. */ >>> + bool matches_const_scalar_int_p = false; >>> + bool matches_other_p = false; >>> + for (int i = 0; i < NUM_RTX_CODE; ++i) >>> + if (p->codes[i]) >>> + switch (i) >>> + { >>> + CASE_CONST_SCALAR_INT: >>> + matches_const_scalar_int_p = true; >>> + break; >>> + >>> + default: >>> + matches_other_p = true; >>> + break; >>> + } >>> + >>> + /* There's no need for a mode check if the predicate only accepts >>> + constant integers. The code checks in the predicate are enough >>> + to establish that the mode is VOIDmode. >>> + >>> + Note that the predicate itself should check whether a scalar >>> + integer is in range of the given mode. */ >>> + if (!matches_other_p && !p->codes[CONST_DOUBLE]) >>> + return; >> >> I think perhaps it would be cleaner to not use CASE_CONST_SCALAR_INT, >> and then do >> >> switch (i) >> { >> case CONST_INT: >> case CONST_WIDE_INT: >> matches_const_scalar_int_p = true; >> break; >> >> case CONST_DOUBLE: >> if (!TARGET_SUPPORTS_WIDE_INT) >> matches_const_scalar_int_p = true; >> matches_other_p = true; >> break; >> >> default: >> matches_other_p = true; >> break; >> } >> >> if (!matches_other_p) >> return; >> >> Otherwise ok. > > Ah, yeah, that's better. Here's what I applied after testing on > x86_64-linux-gnu. This broke the build for !TARGET_SUPPORTS_WIDE_INT because the default definition was in the wrong part of defaults.h: #ifdef GCC_INSN_FLAGS_H /* Dependent default target macro definitions This section of defaults.h defines target macros that depend on generated headers. This is a bit awkward: We want to put all default definitions for target macros in defaults.h, but some of the defaults depend on the HAVE_* flags defines of insn-flags.h. But insn-flags.h is not always included by files that do include defaults.h. Fortunately, the default macro definitions that depend on the HAVE_* macros are also the ones that will only be used inside GCC itself, i.e. not in the gen* programs or in target objects like libgcc. Obviously, it would be best to keep this section of defaults.h as small as possible, by converting the macros defined below to target hooks or functions. */ ... #endif /* GCC_INSN_FLAGS_H */ The macro is supposed to be a configuration invariant and so there's no reason for it depend on generated headers. The same goes for SWITCHABLE_TARGET (oops). Tested by making sure that aarch64-linux-gnu builds again. Committed as obvious. Thanks, Richard gcc/ * defaults.h (SWITCHABLE_TARGET, TARGET_SUPPORTS_WIDE_INT): Move out of GCC_INSN_FLAGS_H block. Index: gcc/defaults.h =================================================================== --- gcc/defaults.h 2015-06-03 10:21:20.247590112 +0100 +++ gcc/defaults.h 2015-06-03 10:21:27.015513249 +0100 @@ -1253,6 +1253,18 @@ #define STACK_PUSH_CODE PRE_INC # define DEFAULT_FLAG_PIE 0 #endif +#ifndef SWITCHABLE_TARGET +#define SWITCHABLE_TARGET 0 +#endif + +/* If the target supports integers that are wider than two + HOST_WIDE_INTs on the host compiler, then the target should define + TARGET_SUPPORTS_WIDE_INT and make the appropriate fixups. + Otherwise the compiler really is not robust. */ +#ifndef TARGET_SUPPORTS_WIDE_INT +#define TARGET_SUPPORTS_WIDE_INT 0 +#endif + #ifdef GCC_INSN_FLAGS_H /* Dependent default target macro definitions @@ -1414,18 +1426,6 @@ #define STACK_CHECK_MAX_VAR_SIZE (STACK_ #define TARGET_VTABLE_USES_DESCRIPTORS 0 #endif -#ifndef SWITCHABLE_TARGET -#define SWITCHABLE_TARGET 0 -#endif - -/* If the target supports integers that are wider than two - HOST_WIDE_INTs on the host compiler, then the target should define - TARGET_SUPPORTS_WIDE_INT and make the appropriate fixups. - Otherwise the compiler really is not robust. */ -#ifndef TARGET_SUPPORTS_WIDE_INT -#define TARGET_SUPPORTS_WIDE_INT 0 -#endif - #ifndef HAVE_simple_return #define HAVE_simple_return 0 static inline rtx ^ permalink raw reply [flat|nested] 30+ messages in thread
end of thread, other threads:[~2015-06-03 9:33 UTC | newest] Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-27 10:20 Mostly rewrite genrecog Richard Sandiford 2015-04-28 23:18 ` Jeff Law 2015-04-29 13:59 ` Richard Sandiford 2015-04-29 8:52 ` Eric Botcazou 2015-04-29 13:51 ` Richard Sandiford 2015-04-30 10:44 ` Eric Botcazou 2015-04-30 6:54 ` Bin.Cheng 2015-04-30 11:58 ` Richard Sandiford 2015-04-30 7:17 ` Andreas Schwab 2015-04-30 7:58 ` Richard Sandiford 2015-04-30 12:10 ` Andreas Schwab 2015-04-30 12:33 ` Richard Biener 2015-04-30 16:27 ` Richard Sandiford 2015-05-01 12:42 ` Richard Sandiford 2015-05-01 13:57 ` Jeff Law 2015-05-07 8:59 ` [nvptx] " Thomas Schwinge 2015-05-07 9:14 ` Jakub Jelinek 2015-05-21 8:09 ` Thomas Schwinge 2015-05-21 11:57 ` Bernd Schmidt 2015-05-08 9:10 ` genrecog: Address -Wsign-compare diagnostics (was: Mostly rewrite genrecog) Thomas Schwinge 2015-05-08 14:43 ` genrecog: Address -Wsign-compare diagnostics Richard Sandiford 2015-05-08 18:39 ` Jeff Law 2015-05-16 8:13 ` Mostly rewrite genrecog Andreas Krebbel 2015-05-17 22:05 ` Richard Sandiford 2015-05-22 16:14 ` Andreas Krebbel 2015-05-22 16:32 ` Richard Sandiford 2015-05-29 17:39 ` RFA: Fix mode checks for possibly-constant predicates Richard Sandiford 2015-05-29 19:27 ` Richard Henderson 2015-06-03 7:23 ` Richard Sandiford 2015-06-03 9:39 ` Richard Sandiford
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).