| Message ID | d26447a6-c97a-149d-393a-a0f15a3d8c0a@t-online.de |
|---|---|
| Headers | show |
| Series | Eliminate cc0 from m68k | expand |
This tidies up a few spots in the m68k backend in preparation for the large patch to follow. This is purely for review purposes: this patch has not been tested independently, and will be committed together with the following one. Noteworthy changes: Some patterns and peepholes were unified through mode iterators. The m68k_subword_comparison_operator predicate was adapted to also work with SImode. There are already scc_di patterns, so there is no need to generate a cc0 set/use pair in cstoredi. Without HAVE_cc0, combine sometimes substitutes a stack push into the destination of a divmod instruction, and then gets confused because it doesn't seem to expect it in a PARALLEL. Since the instruction only works on registers anyway, use register_operand. There are patterns that use register_operand with "do" constraints which allow memory. This works at reload time, but the instruction can not be rerecognized later on. This becomes a problem if such operands occur in a jump instruction, as subsequent passes will try to redirect branches and thus attempt to rerecognize the pattern. movqi/movhi do not accept constants that are not CONST_INT. The code to output them would not set flags correctly and was changed to gcc_unreachable. Comments were added to some patterns which are not being generated due to incorrect tests/predicates. Fixing these is out of scope for this work, but the problems are at least documented. All the passes working on conditional traps seem to assume const_true_rtx is used for unconditional ones, rather than const1_rtx. Bernd
This achieves the conversion by using combined cbranch/cstore patterns, and using a mechanism similar to the cc_status tracking to elide certain comparisons. Unlike cc_status, this is opt-in and requires a flags_valid attribute to be set for suitable instructions. Due to lack of test hardware, this conversion is omitted for a number of coldfire patterns as opposed to normal m68k. For DImode comparisons, scc_di and beq0_di/bne0_di patterns already existed and are reused. The bgt0_di/ble0_di patterns are replaced with expander code to test just the high word, along with some smarts in m68k_find_flags_value. Bernd
Once more with patch. Bernd
On Wed, Nov 13, 2019 at 02:04:59PM +0100, Bernd Schmidt wrote:
> This is a set of patches to convert m68k so that it no longer uses cc0.
I tried this out with a kernel build (just the defconfig). First problem
was patch 4 doesn't apply, it has white-space damage. It's small, I fixed
that up manually. But then I hit
during RTL pass: jump2
/home/segher/src/kernel/fs/binfmt_elf.c: In function 'elf_core_dump':
/home/segher/src/kernel/fs/binfmt_elf.c:2409:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1290
0x102c3c2f patch_jump_insn
/home/segher/src/gcc/gcc/cfgrtl.c:1290
0x102c3df3 redirect_branch_edge
/home/segher/src/gcc/gcc/cfgrtl.c:1317
0x102c442b rtl_redirect_edge_and_branch
/home/segher/src/gcc/gcc/cfgrtl.c:1450
0x102ad04f redirect_edge_and_branch(edge_def*, basic_block_def*)
/home/segher/src/gcc/gcc/cfghooks.c:373
0x10dbb517 try_forward_edges
/home/segher/src/gcc/gcc/cfgcleanup.c:562
0x10dbb517 try_optimize_cfg
/home/segher/src/gcc/gcc/cfgcleanup.c:2960
0x10dbb517 cleanup_cfg(int)
/home/segher/src/gcc/gcc/cfgcleanup.c:3174
0x10dbd41f execute
/home/segher/src/gcc/gcc/cfgcleanup.c:3353
Can you reproduce that?
Segher
On 11/13/19 7:16 PM, Segher Boessenkool wrote: > I tried this out with a kernel build (just the defconfig). > during RTL pass: jump2 > /home/segher/src/kernel/fs/binfmt_elf.c: In function 'elf_core_dump': > /home/segher/src/kernel/fs/binfmt_elf.c:2409:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1290 > Can you reproduce that? Yes. It's actually an issue I spotted at one point, but I thought to myself "I'll just leave it, I want to make the minimum amount of changes". Should have thought harder. The constraints for comparison patterns in m68k.md allow constants as the first operand of a comparison. They also use nonimmediate_operand. Hence, the internal error you saw when something tries to rerecognize the instruction. The following should fix it, but it's only very lightly tested so far. I'll merge it into patch 2. Bernd
On Wed, Nov 13, 2019 at 07:57:58PM +0100, Bernd Schmidt wrote: > On 11/13/19 7:16 PM, Segher Boessenkool wrote: > > I tried this out with a kernel build (just the defconfig). > > > during RTL pass: jump2 > > /home/segher/src/kernel/fs/binfmt_elf.c: In function 'elf_core_dump': > > /home/segher/src/kernel/fs/binfmt_elf.c:2409:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1290 > > > Can you reproduce that? > > Yes. It's actually an issue I spotted at one point, but I thought to > myself "I'll just leave it, I want to make the minimum amount of > changes". Should have thought harder. > > The constraints for comparison patterns in m68k.md allow constants as > the first operand of a comparison. They also use nonimmediate_operand. > Hence, the internal error you saw when something tries to rerecognize > the instruction. > > The following should fix it, but it's only very lightly tested so far. > I'll merge it into patch 2. It works on the kernel build, at least. No idea if this *runs*, I just built it ;-) Sizes, R0 is trunk, R1 is with your patches: R0 R1 m68k 3720557 3721245 (that is 100.018%, looks great!) Segher
On 11/13/19 6:04 AM, Bernd Schmidt wrote: > This is a set of patches to convert m68k so that it no longer uses cc0. > The approach is to combine cc0 setter/user pairs into cbranch and cstore > patterns. It does not expose the flag register directly. Since m68k is a > target that is not under active development, and probably receives very > limited testing, I felt it was important to make it generate as close to > the same code as previously. Also, given that the target clobbers the > flags for pretty much every move, it seems unlikely that there's much > value to be had from anything more complex. Trying to model every > instruction's effect on the flags would be too error-prone for not > nearly enough gain. I tend to agree. My only worry would be introducing a new way to deal with cc0. But I'll certainly look at the changes with an open mind. It might play well with ideas I had while looking at the H8 port which has a very similar model. > > The cc0 machinery allows for eliminating unnecessary comparisons by > examining the effect instructions have on the flags registers. I have > replicated that mechanism with a relatively modest amount of code based > on a final_postscan_insn hook, but it is now opt-in: an instruction > pattern can set the "flags_valid" attribute to a number of possible > values to indicate what effect it has. That should be more reliable (try > git log m68k.md to see recent sprinkling of CC_STATUS_INIT to squash > bugs with the previous mechanism). Yea, sounds like a reimplementation of the tst elimination bits, but buried in the backend. Given the choice of dropping the port or burying this kind of stuff in there, I'd lean towards accepting the latter. > We can remember either values where the flags indicate a comparison > against zero (after practically all arithmetic and move insns), or > alternatively record two comparison operands to eliminate identical > compares. I stopped adding optimizations once I found it hard to find > any meaningful differences in generated code. In particular, the > m68k.exp tests which verify that these optimizations are performed all > still pass. Yea. One of the things I was pondering was dropping many/most of the combiner patterns then only adding those back which were clearly still important. I have a strong suspicion we have *many* unnecessary patterns. Of course finding those may be more work than its worth. > > Testing was done with the qemu-system-m68k/debian combination. I do not > have access to Coldfire hardware, and I tried to be somewhat > conservative, for example by not adding "flags_valid" everywhere it > would probably be possible. For someone with access to the hardware, it > should be trivial to add such attributes and test that everything still > works. I'm happy to add your patch to my tester. It'll verify the compiler bootstraps and can build glibc & the kernel. Jeff ps. On a more personal note -- good to hear from you Bernd. I hope things are going well.
On 11/13/19 6:08 AM, Bernd Schmidt wrote: > This tidies up a few spots in the m68k backend in preparation for the > large patch to follow. This is purely for review purposes: this patch > has not been tested independently, and will be committed together with > the following one. > > Noteworthy changes: > > Some patterns and peepholes were unified through mode iterators. The > m68k_subword_comparison_operator predicate was adapted to also work with > SImode. > > There are already scc_di patterns, so there is no need to generate a cc0 > set/use pair in cstoredi. > > Without HAVE_cc0, combine sometimes substitutes a stack push into the > destination of a divmod instruction, and then gets confused because it > doesn't seem to expect it in a PARALLEL. Since the instruction only > works on registers anyway, use register_operand. > > There are patterns that use register_operand with "do" constraints which > allow memory. This works at reload time, but the instruction can not be > rerecognized later on. This becomes a problem if such operands occur in > a jump instruction, as subsequent passes will try to redirect branches > and thus attempt to rerecognize the pattern. > > movqi/movhi do not accept constants that are not CONST_INT. The code to > output them would not set flags correctly and was changed to > gcc_unreachable. > > Comments were added to some patterns which are not being generated due > to incorrect tests/predicates. Fixing these is out of scope for this > work, but the problems are at least documented. > > All the passes working on conditional traps seem to assume > const_true_rtx is used for unconditional ones, rather than const1_rtx. > > > Bernd > > > m68k-1.diff > > * config/m68k/m68k.c (output_move_himode, output_move_qimode): > Replace code for non-CONST_INT constants with gcc_unreachable. > * config/m68k/m68k.md (cbranchdi): Don't generate individual > compare and test. > (CMPMODE): New mode_iterator. > (cbranchsi4, cbranchqi4, cbranchhi4): Replace expanders with > cbranch<mode>4. > (cstoresi4, cstoreqi4, cstorehi4): Replace expanders with > cstore<mode>4. > (cmp<mode>_68881): Remove 'F' constraint from first comparison > operand. > (bit test insns patterns): Use nonimmediate_operand, not > register_operand, for source operands that allow memory in > their constraints. > (divmodsi4, udivmodsi4, divmodhi4 and related unnamed patterns): > Use register_operand, not nonimmediate_operand, for the > destinations. > (DBCC): New mode_iterator. > (dbcc peepholes): Use it to reduce duplication. > (trap): Use const_true_rtx, not const1_rtx. > * config/m68k/predicates.md (m68k_comparison_operand): Renamed > from m68k_subword_comparison_operand and changed to handle > SImode. OK. I'd actually recommend this go ahead and get installed. My tester will bootstrap it overnight. Jeff
On 11/13/19 9:03 PM, Jeff Law wrote: > OK. I'd actually recommend this go ahead and get installed. My tester > will bootstrap it overnight. Alright, let me know how that turns out. What kind of machine do you have for that? Bernd
On 11/13/19 8:35 PM, Jeff Law wrote: > On 11/13/19 6:04 AM, Bernd Schmidt wrote: >> The cc0 machinery allows for eliminating unnecessary comparisons by >> examining the effect instructions have on the flags registers. I have >> replicated that mechanism with a relatively modest amount of code based >> on a final_postscan_insn hook, but it is now opt-in: an instruction >> pattern can set the "flags_valid" attribute to a number of possible >> values to indicate what effect it has. That should be more reliable (try >> git log m68k.md to see recent sprinkling of CC_STATUS_INIT to squash >> bugs with the previous mechanism). > Yea, sounds like a reimplementation of the tst elimination bits, but > buried in the backend. Given the choice of dropping the port or burying > this kind of stuff in there, I'd lean towards accepting the latter. Indeed. Even if we wanted an eventual transition to the tst elimination bits, this is a better starting place than from cc0. r~
Hi Segher! > It works on the kernel build, at least. No idea if this *runs*, I just > built it ;-) I just did that and kernel 5.3.9 built with gcc trunk with Bernd's patches boots fine on qemu-m68k-system. I will test the kernel on a real Amiga later but I'm confident it will work there as well. Thanks to everyone, especially Bernd for helping to make the cc0 transition for m68k happen! Adrian
Here are the results of running the testsuite on m68k-linux: http://gcc.gnu.org/ml/gcc-testresults/2019-11/msg00908.html This is a list of regressions: g++.old-deja/g++.other/dyncast1.C -std=c++14 execution test g++.old-deja/g++.other/dyncast1.C -std=c++17 execution test g++.old-deja/g++.other/dyncast1.C -std=c++2a execution test g++.old-deja/g++.other/dyncast1.C -std=c++98 execution test g++.old-deja/g++.other/dyncast6.C -std=gnu++14 execution test g++.old-deja/g++.other/dyncast6.C -std=gnu++17 execution test g++.old-deja/g++.other/dyncast6.C -std=gnu++2a execution test g++.old-deja/g++.other/dyncast6.C -std=gnu++98 execution test g++.old-deja/g++.other/rttid3.C -std=gnu++14 execution test g++.old-deja/g++.other/rttid3.C -std=gnu++17 execution test g++.old-deja/g++.other/rttid3.C -std=gnu++2a execution test g++.old-deja/g++.other/rttid3.C -std=gnu++98 execution test g++.old-deja/g++.robertl/eb46.C -std=c++14 execution test g++.old-deja/g++.robertl/eb46.C -std=c++17 execution test g++.old-deja/g++.robertl/eb46.C -std=c++2a execution test g++.old-deja/g++.robertl/eb46.C -std=c++98 execution test 18_support/nested_exception/rethrow_if_nested.cc execution test 20_util/function_objects/comparisons_pointer.cc execution test 21_strings/basic_string/inserters_extractors/char/8.cc execution test 22_locale/num_get/get/char/16.cc execution test 24_iterators/istream_iterator/1.cc execution test 25_algorithms/copy_n/50119.cc execution test 27_io/basic_istream/extractors_arithmetic/char/01.cc execution test 27_io/basic_istream/extractors_arithmetic/char/02.cc execution test 27_io/basic_istream/extractors_arithmetic/char/03.cc execution test 27_io/basic_istream/extractors_arithmetic/char/10.cc execution test 27_io/basic_istream/extractors_arithmetic/char/11.cc execution test 27_io/basic_istream/extractors_arithmetic/char/13.cc execution test 27_io/basic_istream/extractors_arithmetic/char/dr696.cc execution test 27_io/basic_istream/seekg/char/8348-1.cc execution test 27_io/basic_istream/seekg/char/8348-2.cc execution test 27_io/basic_istream/tellg/char/8348.cc execution test 27_io/basic_istringstream/assign/1.cc execution test 27_io/basic_istringstream/cons/move.cc execution test 27_io/basic_istringstream/str/char/1.cc execution test 27_io/basic_stringbuf/seekoff/char/1.cc execution test 27_io/basic_stringbuf/seekoff/wchar_t/1.cc execution test 27_io/basic_stringbuf/seekoff/wchar_t/2.cc execution test 27_io/basic_stringstream/assign/1.cc execution test 27_io/filesystem/path/concat/path.cc execution test 27_io/manipulators/standard/char/quoted.cc execution test 27_io/rvalue_streams.cc execution test 28_regex/algorithms/regex_match/awk/cstring_01.cc execution test 28_regex/algorithms/regex_match/ecma/char/57173.cc execution test 28_regex/algorithms/regex_match/ecma/char/68863.cc execution test 28_regex/algorithms/regex_match/ecma/char/backref.cc execution test 28_regex/algorithms/regex_match/ecma/char/emptygroup.cc execution test 28_regex/algorithms/regex_match/ecma/char/hex.cc execution test 28_regex/algorithms/regex_match/extended/cstring_range.cc execution test 28_regex/algorithms/regex_replace/char/basic_replace.cc execution test 28_regex/algorithms/regex_replace/char/dr2213.cc execution test 28_regex/basic_regex/multiple_quantifiers.cc execution test 28_regex/match_results/format.cc execution test 28_regex/regression.cc execution test 28_regex/traits/char/value.cc execution test backward/strstream_move.cc execution test ext/random/k_distribution/operators/serialize.cc execution test ext/random/nakagami_distribution/operators/serialize.cc execution test ext/random/normal_mv_distribution/operators/serialize.cc execution test ext/random/rice_distribution/operators/serialize.cc execution test ext/random/uniform_inside_sphere_distribution/operators/serialize.cc execution test tr1/5_numerical_facilities/random/discard_block/operators/serialize.cc execution test tr1/7_regular_expressions/regex_traits/char/value.cc execution test tr1/7_regular_expressions/regex_traits/wchar_t/value.cc execution test Andreas.
Hi! On 11/15/19 2:45 PM, John Paul Adrian Glaubitz wrote: >> It works on the kernel build, at least. No idea if this *runs*, I just >> built it ;-) > > I just did that and kernel 5.3.9 built with gcc trunk with Bernd's patches > boots fine on qemu-m68k-system. I will test the kernel on a real Amiga > later but I'm confident it will work there as well. Kernel built with the patched gcc-10 also boots fine on my Amiga 4000 68060 :). I'm test building various packages now to see if I can see any other issues but so far I'm very confident that there are no obvious regressions. Adrian
On 11/15/19 2:48 PM, Andreas Schwab wrote: > Here are the results of running the testsuite on m68k-linux: > > http://gcc.gnu.org/ml/gcc-testresults/2019-11/msg00908.html > > This is a list of regressions: Are these with the patch? I'm not seeing any of these in my testing with qemu. Are you on real hardware (and on which hardware?), and can you do anything to help narrow down what's going wrong? Bernd
On Nov 15 2019, Bernd Schmidt wrote: > Are these with the patch? Yes. > Are you on real hardware No, I'm using aranym. Andreas.
On 11/15/19 5:34 PM, Andreas Schwab wrote: > On Nov 15 2019, Bernd Schmidt wrote: > >> Are these with the patch? > > Yes. > >> Are you on real hardware > > No, I'm using aranym. Any chance you could show the command lines from the log files or some other way of reproducing the issue? Bernd
On Nov 15 2019, Bernd Schmidt wrote: > Any chance you could show the command lines from the log files or some > other way of reproducing the issue? Executing on aranym: OMP_NUM_THREADS=2 LD_LIBRARY_PATH=.:/daten/aranym/gcc/gcc-20191115/Build/m68k-linux/./libstdc++-v3/src/.libs:/daten/aranym/gcc/gcc-20191115/Build/m68k-linux/./libstdc++-v3/src/.libs:/daten/aranym/gcc/gcc-20191115/Build/gcc:/daten/aranym/gcc/gcc-20191115/Build/gcc timeout 1200 ./dyncast1.exe (timeout = 300) Executed ./dyncast1.exe, status 1 Output: Error 25 child process exited abnormally FAIL: g++.old-deja/g++.other/dyncast1.C -std=c++17 execution test Andreas.
On 11/15/19 10:58 PM, Andreas Schwab wrote: > On Nov 15 2019, Bernd Schmidt wrote: > >> Any chance you could show the command lines from the log files or some >> other way of reproducing the issue? > > Executing on aranym: OMP_NUM_THREADS=2 LD_LIBRARY_PATH=.:/daten/aranym/gcc/gcc-20191115/Build/m68k-linux/./libstdc++-v3/src/.libs:/daten/aranym/gcc/gcc-20191115/Build/m68k-linux/./libstdc++-v3/src/.libs:/daten/aranym/gcc/gcc-20191115/Build/gcc:/daten/aranym/gcc/gcc-20191115/Build/gcc timeout 1200 ./dyncast1.exe (timeout = 300) > Executed ./dyncast1.exe, status 1 > Output: Error 25 > child process exited abnormally > FAIL: g++.old-deja/g++.other/dyncast1.C -std=c++17 execution test I meant the compiler command line of course... for any -mcpu flags that might differ from my test run. Bernd
On Nov 15 2019, Bernd Schmidt wrote: > I meant the compiler command line of course... for any -mcpu flags that > might differ from my test run. There are none. Andreas.
On 11/15/19 11:50 PM, Andreas Schwab wrote: > On Nov 15 2019, Bernd Schmidt wrote: > >> I meant the compiler command line of course... for any -mcpu flags that >> might differ from my test run. > > There are none. Well, there has to be some difference between what you are doing and what I am doing, because: Running /local/src/egcs/git/gcc/testsuite/g++.old-deja/old-deja.exp ... === g++ Summary === # of expected passes 26826 # of expected failures 82 # of unsupported tests 157 /local/src/egcs/bm68k-test/gcc/xg++ version 10.0.0 20191101 (experimental) (GCC) Is there anything you think you can give me to help reproduce this? Before/after assembly files, generated with -fverbose-asm? Bernd
On Nov 16 2019, Bernd Schmidt wrote: > Well, there has to be some difference between what you are doing and > what I am doing, because: > > Running /local/src/egcs/git/gcc/testsuite/g++.old-deja/old-deja.exp ... > > === g++ Summary === > > # of expected passes 26826 > # of expected failures 82 > # of unsupported tests 157 > /local/src/egcs/bm68k-test/gcc/xg++ version 10.0.0 20191101 > (experimental) (GCC) === g++ Summary === # of expected passes 170041 # of unexpected failures 74 # of expected failures 708 # of unresolved testcases 2 # of unsupported tests 7419 /daten/aranym/gcc/gcc-20191115/Build/gcc/xg++ version 10.0.0 20191114 (experimental) [trunk revision 278266] (GCC) Andreas.
Running /daten/aranym/gcc/gcc-20191116/gcc/testsuite/g++.old-deja/old-deja.exp ...
FAIL: g++.old-deja/g++.other/dyncast1.C -std=c++98 execution test
FAIL: g++.old-deja/g++.other/dyncast1.C -std=c++14 execution test
FAIL: g++.old-deja/g++.other/dyncast1.C -std=c++17 execution test
FAIL: g++.old-deja/g++.other/dyncast1.C -std=c++2a execution test
FAIL: g++.old-deja/g++.other/dyncast6.C -std=gnu++98 execution test
FAIL: g++.old-deja/g++.other/dyncast6.C -std=gnu++14 execution test
FAIL: g++.old-deja/g++.other/dyncast6.C -std=gnu++17 execution test
FAIL: g++.old-deja/g++.other/dyncast6.C -std=gnu++2a execution test
FAIL: g++.old-deja/g++.other/rttid3.C -std=gnu++98 execution test
FAIL: g++.old-deja/g++.other/rttid3.C -std=gnu++14 execution test
FAIL: g++.old-deja/g++.other/rttid3.C -std=gnu++17 execution test
FAIL: g++.old-deja/g++.other/rttid3.C -std=gnu++2a execution test
FAIL: g++.old-deja/g++.robertl/eb46.C -std=c++98 execution test
FAIL: g++.old-deja/g++.robertl/eb46.C -std=c++14 execution test
FAIL: g++.old-deja/g++.robertl/eb46.C -std=c++17 execution test
FAIL: g++.old-deja/g++.robertl/eb46.C -std=c++2a execution test
=== g++ Summary ===
# of expected passes 26803
# of unexpected failures 16
# of expected failures 82
# of unsupported tests 157
/daten/aranym/gcc/gcc-20191116/Build/gcc/xg++ version 10.0.0 20191115 (experimental) [trunk revision 278320] (GCC)
Andreas.
Hi Andreas! > === g++ Summary === > > # of expected passes 26803 > # of unexpected failures 16 > # of expected failures 82 > # of unsupported tests 157 > /daten/aranym/gcc/gcc-20191116/Build/gcc/xg++ version 10.0.0 20191115 (experimental) [trunk revision 278320] (GCC) Is this testsuite run with or without Bernd's patches? If yes, I think it would be a good sign that the patches are good for submission. I will try to get the patches backported to gcc-9 so that we can start building Debian packages with a patched gcc-9 which may help finding more regressions. Adrian
On 11/16/19 9:18 AM, Andreas Schwab wrote: > On Nov 16 2019, Bernd Schmidt wrote: > >> Well, there has to be some difference between what you are doing and >> what I am doing, because: >> >> Running /local/src/egcs/git/gcc/testsuite/g++.old-deja/old-deja.exp ... >> >> === g++ Summary === >> >> # of expected passes 26826 >> # of expected failures 82 >> # of unsupported tests 157 >> /local/src/egcs/bm68k-test/gcc/xg++ version 10.0.0 20191101 >> (experimental) (GCC) > > === g++ Summary === > > # of expected passes 170041 > # of unexpected failures 74 > # of expected failures 708 > # of unresolved testcases 2 > # of unsupported tests 7419 > /daten/aranym/gcc/gcc-20191115/Build/gcc/xg++ version 10.0.0 20191114 (experimental) [trunk revision 278266] (GCC) Once again, that doesn't help me track things down. A bug report without instructions to reproduce is useless. Could you please provide me the generated assembly files with -fverbose-asm that I asked for? Bernd
On 11/13/19 6:23 AM, Bernd Schmidt wrote: > Once more with patch. > > > Bernd > > > m68k-2.diff > > PR target/91851 > * config/m68k/m68k-protos.h (output-dbcc_and_branch): Adjust > declaration. > (m68k_init_cc): New declaration. > (m68k_output_compare_di, m68k_output_compare_si, > m68k_output_compare_hi, m68k_output_compare_qi, > m68k_output_compare_fp, m68k_output_btst, m68k_output_bftst, > m68k_find_flags_value, m68k_output_scc, m68k_output_scc_float, > m68k_output_branch_integer, m68k_output_branch_integer_rev. > m68k_output_branch_float, m68k_output_branch_float_rev): > Likewise. > (valid_dbcc_comparison_p_2, flags_in_68881, > output_btst): Remove declaration. > * config/m68k/m68k.c (INCLDUE_STRING): Define. > (TARGET_ASM_FINAL_POSTSCAN_INSN): Define. > (valid_dbcc_comparison_p_2, flags_in_68881): Delete functions. > (flags_compare_op0, flags_compare_op1, flags_operand1, > flags_operand2, flags_valid): New static variables. > (m68k_find_flags_value, m68k_init_cc): New functions. > (handle_flags_for_move, m68k_asm_final_postscan_insn, > remember_compare_flags): New static functions. > (output_dbcc_and_branch): New argument CODE. Use it, and add > PLUS and MINUS to the possible codes. All callers changed. > (m68k_output_btst): Renamed from output_btst. Remove OPERANDS > and INSN arguments, add CODE arg. Return the comparison code > to use. All callers changed. Use CODE instead of > next_insn_tests_no_inequality, and replace cc_status management > with changing the return code. > (m68k_rtx_costs): Instead of testing for COMPARE, test for > RTX_COMPARE or RTX_COMM_COMPARE. > (output_move_simode, output_move_qimode): Call > handle_flags_for_move. > (notice_update_cc): Delete function. > (m68k_output_bftst, m68k_output_compare_di, m68k_output_compare_si, > m68k_output_compare_hi, m68k_output_compare_qi, > m68k_output_compare_fp, m68k_output_branch_integer, > m68k_output_branch_integer_rev, m68k_output_scc, > m68k_output_branch_float, m68k_output_branch_float_rev, > m68k_output_scc_float): New functions. > (output_andsi3, output_iorsi3, output_xorsi3): Call CC_STATUS_INIT > once at the start, and set flags_valid and flags_operand1 if the > flags are usable. > * config/m68k/m68k.h (CC_IN_68881, NOTICE_UPDATE_CC, > CC_OVERFLOW_UNUSABLE, CC_NO_CARRY, OUTPUT_JUMP): Remove > definitions. > (CC_STATUS_INIT): Define. > * config/m68k/m68k.md (flags_valid): New define_attr. > (tstdi, tstsi_internal_68020_cf, tstsi_internal, tsthi_internal, > tstqi_internal, tst<mode>_68881, tst<mode>_cf, cmpdi_internal, > cmpdi, unnamed cmpsi/cmphi/cmpqi patterns, cmpsi_cf, > cmp<mode>_68881, cmp<mode>_cf, unnamed btst patterns, > tst_bftst_reg, tst_bftst_reg, unnamed scc patterns, scc, > sls, sordered_1, sunordered_1, suneq_1, sunge_1, sungt_1, > sunle_1, sunlt_1, sltgt_1, fsogt_1, fsoge_1, fsolt_1, fsole_1, > bge0_di, blt0_di, beq, bne, bgt, bgtu, blt, bltu, bge, bgeu, > ble, bleu, bordered, bunordered, buneq, bunge, bungt, bunle, > bunlt, bltgt, beq_rev, bne_rev, bgt_rev, bgtu_rev, > blt_rev, bltu_rev, bge_rev, bgeu_rev, ble_rev, bleu_rev, > bordered_rev, bunordered_rev, buneq_rev, bunge_rv, bungt_rev, > bunle_rev, bunlt_rev, bltgt_rev, ctrapdi4, ctrapsi4, ctraphi4, > ctrapqi4, conditional_trap): Delete patterns. > (cbranchdi4_insn): New pattern. > (cbranchdi4): Don't generate cc0 patterns. When testing LT or GE, > test high part only. When testing EQ or NE, generate beq0_di > and bne0_di patterns directly. > (cstoredi4): When testing LT or GE, test high part only. > (both sets of cbranch<mode>4, cstore<mode>4): Don't generate cc0 > patterns. > (scc0_constraints, cmp1_constraints, cmp2_constraints, > scc0_cf_constraints, cmp1_cf_constraints, cmp2_cf_constraints, > cmp2_cf_predicate): New define_mode_attrs. > (cbranch<mode>4_insn, cbranch<mode>4_insn_rev, > cbranch<mode>4_insn_cf, cbranch<mode>4_insn_cf_rev, > cstore<mode>4_insn, cstore<mode>4_insn_cf for integer modes) > New patterns. > (cbranch<mode>4_insn_68881, cbranch<mode>4_insn_rev_68881): > (cbranch<mode>4_insn_cf, cbranch<mode>4_insn_rev_cf, > cstore<mode>4_insn_68881, cstore<mode>4_insn_cf for FP): > New patterns. > (cbranchsi4_btst_mem_insn, cbranchsi4_btst_reg_insn, > cbranchsi4_btst_mem_insn_1, cbranchsi4_btst_reg_insn_1): > Likewise. > (BTST): New define_mode_iterator. > (btst_predicate, btst_constraint, btst_range): New > define_mode_attrs. > (cbranch_bftst<mode>_insn, cstore_bftst<mode>_insn): New > patterns. > (movsi_m68k_movsi_m68k2, movsi_cf, unnamed movstrict patterns, > unnamed movhi and movqi patterns, unnamed movsf, movdf and movxf > patterns): Set attr "flags_valid". > (truncsiqi2, trunchiqi2, truncsihi2): Remove manual CC_STATUS > management. Set attr "flags_valid". > (extendsidi2, extendplussidi, unnamed float_extendsfdf pattern, > extendsfdf2_cf, fix_truncdfsi2, fix_truncdfhi2, fix_truncdfqi2, > addi_sexthishl32, adddi_dilshr32, adddi_dilshr32_cf, > addi_dishl32, subdi_sexthishl32, subdi_dishl32, subdi3): Remove > manual CC_STATUS management. > (addsi3_internal, addhi3, addqi3, subsi3, subhi3, subqi3, > unnamed strict_lowpart subhi and subqi patterns): Set attr > "flags_valid". > (unnamed strict_lowpart addhi3 and addqi3 patterns): Likewise. > Remove code to operate on address regs and assert the case > does not occur. > (unnamed mulsidi patterns, divmodhi4, udivmodhi4): Remove > manual CC_STATUS_INIT. > (andsi3_internal, andhi3, andqi3, iorsi3_internal, iorhi3, iorqi3, > xorsi3_internal, xorhi3, xorqi3, negsi2_internal, > negsi2_5200, neghi2, negqi2, one_cmplsi2_internal, one_cmplhi2, > one_cmplqi2, unnamed strict_lowpart patterns > for andhi, andqi, iorhi, iorqi, xorhi, xorqi, neghi, negqi, > one_cmplhi and one_cmplqi): Set attr "flags_valid". > (iorsi_zext_ashl16, iorsi_zext): Remove manual CC_STATUS_INIT. > (ashldi_sexthi, ashlsi_16, ashlsi_17_24): Remove manual > CC_STATUS_INIT. > (ashlsi3, ashlhi3, ashlqi3, ashrsi3, ashrhi3, ashrqi3, lshrsi3, > lshrhi3, shrqi3, rotlsi3, rotlhi3, rotlhi3_lowpart, rotlqi3, > rotlqi3_lowpart, rotrsi3, rotrhi3, rotrhi_lowpart, rotrqi3, > unnamed strict_low_part patterns for HI and > QI versions): Set attr "flags_valid". > (bsetmemqi, bsetmemqi_ext, bsetdreg, bchgdreg, bclrdreg, > bclrmemqi, extzv_8_16_reg, extzv_bfextu_mem, insv_bfchg_mem, > insv_bfclr_mem, insv_bfset_mem, extv_bfextu_reg, > insv_bfclr_reg, insv_bfset_reg, dbne_hi, dbne_si, dbge_hi, > dbge_si, extendsfxf2, extenddfxf2, ): Remove manual cc_status management. > (various unnamed peepholes): Adjust compare/branch sequences > for new cbranch patterns. > (dbcc peepholes): Likewise, and output the comparison here > as well. > * config/m68k/predicates.md (valid_dbcc_comparison_p): Delete. > (fp_src_operand): Allow constant zero. > (address_reg_operand): New predicate. > > * rtl.h (inequality_comparisons_p): Remove declaration. > * recog.h (next_insn_tests_no_inequality): Likewise. > * rtlanal.c (inequality_comparisons_p): Delete function. > * recog.c (next_insn_tests_no_inequality): Likewise. My inclination is to ACK this as-is and let us iterate on any bugs that pop up. Bernd has a long history with GCC and I absolutely trust his ability, judgment and commitment to addressing any issues. While scanning this patch I did notice the introduction of CC_STATUS_INIT in output_{and,ior,xor}si. You might want to check that. So unless there's objections over the next say 48-72 hrs, let's get the kit in and we can iterate if there's further issues that need resolving. Jeff
On 11/14/19 5:23 AM, Bernd Schmidt wrote: > On 11/13/19 9:03 PM, Jeff Law wrote: >> OK. I'd actually recommend this go ahead and get installed. My tester >> will bootstrap it overnight. > > Alright, let me know how that turns out. What kind of machine do you > have for that? Sorry I should have been clearer. I expected you to commit the first patch and my tester would have picked it up automatically. Regrardless, I put the first patch into my magic testing directory, so it'll get bootstrapped later today. jeff
On 11/14/19 5:23 AM, Bernd Schmidt wrote: > On 11/13/19 9:03 PM, Jeff Law wrote: >> OK. I'd actually recommend this go ahead and get installed. My tester >> will bootstrap it overnight. > > Alright, let me know how that turns out. What kind of machine do you > have for that? So patch#1 had a minor problem applying to the top of the trunk, but it was easily resolved by hand. GCC bootstrapped & built glibc and was building the kernel when the beaker machine either crashed or got reclaimed by beaker. So after ~14 hours we know it was working reasonably well (as expected), but don't have full confirmation. jeff
(Apologies to Jeff who's getting this twice because I didn't hit reply-all the first time.) On 11/17/19 6:56 PM, Jeff Law wrote: > While scanning this patch I did notice the introduction of > CC_STATUS_INIT in output_{and,ior,xor}si. You might want to check that. That is intentional. CC_STATUS_INIT can be used without cc0, and there's precedent in (IIRC) ARM. We need it to get final to tell us when it encounters a label - those don't make it to the postscan_insn hook. > So unless there's objections over the next say 48-72 hrs, let's get the > kit in and we can iterate if there's further issues that need resolving. Cool. I'll wait for a final confirmation. Bernd
On 11/18/19 9:51 AM, Bernd Schmidt wrote: > (Apologies to Jeff who's getting this twice because I didn't hit > reply-all the first time.) > > On 11/17/19 6:56 PM, Jeff Law wrote: > >> While scanning this patch I did notice the introduction of >> CC_STATUS_INIT in output_{and,ior,xor}si. You might want to check that. > > That is intentional. CC_STATUS_INIT can be used without cc0, and there's > precedent in (IIRC) ARM. We need it to get final to tell us when it > encounters a label - those don't make it to the postscan_insn hook. OK. Just saw it and found it a bit odd. No worries if it was intentional. jeff
Hi Jeff! > On 11/13/19 6:23 AM, Bernd Schmidt wrote: >> Once more with patch. >> >> >> Bernd >> >> >> m68k-2.diff >> >> PR target/91851 >> * config/m68k/m68k-protos.h (output-dbcc_and_branch): Adjust >> declaration. >> (m68k_init_cc): New declaration. >> (m68k_output_compare_di, m68k_output_compare_si, >> m68k_output_compare_hi, m68k_output_compare_qi, >> m68k_output_compare_fp, m68k_output_btst, m68k_output_bftst, >> m68k_find_flags_value, m68k_output_scc, m68k_output_scc_float, >> m68k_output_branch_integer, m68k_output_branch_integer_rev. >> m68k_output_branch_float, m68k_output_branch_float_rev): >> Likewise. >> (valid_dbcc_comparison_p_2, flags_in_68881, >> output_btst): Remove declaration. >> * config/m68k/m68k.c (INCLDUE_STRING): Define. >> (TARGET_ASM_FINAL_POSTSCAN_INSN): Define. >> (valid_dbcc_comparison_p_2, flags_in_68881): Delete functions. >> (flags_compare_op0, flags_compare_op1, flags_operand1, >> flags_operand2, flags_valid): New static variables. >> (m68k_find_flags_value, m68k_init_cc): New functions. >> (handle_flags_for_move, m68k_asm_final_postscan_insn, >> remember_compare_flags): New static functions. >> (output_dbcc_and_branch): New argument CODE. Use it, and add >> PLUS and MINUS to the possible codes. All callers changed. >> (m68k_output_btst): Renamed from output_btst. Remove OPERANDS >> and INSN arguments, add CODE arg. Return the comparison code >> to use. All callers changed. Use CODE instead of >> next_insn_tests_no_inequality, and replace cc_status management >> with changing the return code. >> (m68k_rtx_costs): Instead of testing for COMPARE, test for >> RTX_COMPARE or RTX_COMM_COMPARE. >> (output_move_simode, output_move_qimode): Call >> handle_flags_for_move. >> (notice_update_cc): Delete function. >> (m68k_output_bftst, m68k_output_compare_di, m68k_output_compare_si, >> m68k_output_compare_hi, m68k_output_compare_qi, >> m68k_output_compare_fp, m68k_output_branch_integer, >> m68k_output_branch_integer_rev, m68k_output_scc, >> m68k_output_branch_float, m68k_output_branch_float_rev, >> m68k_output_scc_float): New functions. >> (output_andsi3, output_iorsi3, output_xorsi3): Call CC_STATUS_INIT >> once at the start, and set flags_valid and flags_operand1 if the >> flags are usable. >> * config/m68k/m68k.h (CC_IN_68881, NOTICE_UPDATE_CC, >> CC_OVERFLOW_UNUSABLE, CC_NO_CARRY, OUTPUT_JUMP): Remove >> definitions. >> (CC_STATUS_INIT): Define. >> * config/m68k/m68k.md (flags_valid): New define_attr. >> (tstdi, tstsi_internal_68020_cf, tstsi_internal, tsthi_internal, >> tstqi_internal, tst<mode>_68881, tst<mode>_cf, cmpdi_internal, >> cmpdi, unnamed cmpsi/cmphi/cmpqi patterns, cmpsi_cf, >> cmp<mode>_68881, cmp<mode>_cf, unnamed btst patterns, >> tst_bftst_reg, tst_bftst_reg, unnamed scc patterns, scc, >> sls, sordered_1, sunordered_1, suneq_1, sunge_1, sungt_1, >> sunle_1, sunlt_1, sltgt_1, fsogt_1, fsoge_1, fsolt_1, fsole_1, >> bge0_di, blt0_di, beq, bne, bgt, bgtu, blt, bltu, bge, bgeu, >> ble, bleu, bordered, bunordered, buneq, bunge, bungt, bunle, >> bunlt, bltgt, beq_rev, bne_rev, bgt_rev, bgtu_rev, >> blt_rev, bltu_rev, bge_rev, bgeu_rev, ble_rev, bleu_rev, >> bordered_rev, bunordered_rev, buneq_rev, bunge_rv, bungt_rev, >> bunle_rev, bunlt_rev, bltgt_rev, ctrapdi4, ctrapsi4, ctraphi4, >> ctrapqi4, conditional_trap): Delete patterns. >> (cbranchdi4_insn): New pattern. >> (cbranchdi4): Don't generate cc0 patterns. When testing LT or GE, >> test high part only. When testing EQ or NE, generate beq0_di >> and bne0_di patterns directly. >> (cstoredi4): When testing LT or GE, test high part only. >> (both sets of cbranch<mode>4, cstore<mode>4): Don't generate cc0 >> patterns. >> (scc0_constraints, cmp1_constraints, cmp2_constraints, >> scc0_cf_constraints, cmp1_cf_constraints, cmp2_cf_constraints, >> cmp2_cf_predicate): New define_mode_attrs. >> (cbranch<mode>4_insn, cbranch<mode>4_insn_rev, >> cbranch<mode>4_insn_cf, cbranch<mode>4_insn_cf_rev, >> cstore<mode>4_insn, cstore<mode>4_insn_cf for integer modes) >> New patterns. >> (cbranch<mode>4_insn_68881, cbranch<mode>4_insn_rev_68881): >> (cbranch<mode>4_insn_cf, cbranch<mode>4_insn_rev_cf, >> cstore<mode>4_insn_68881, cstore<mode>4_insn_cf for FP): >> New patterns. >> (cbranchsi4_btst_mem_insn, cbranchsi4_btst_reg_insn, >> cbranchsi4_btst_mem_insn_1, cbranchsi4_btst_reg_insn_1): >> Likewise. >> (BTST): New define_mode_iterator. >> (btst_predicate, btst_constraint, btst_range): New >> define_mode_attrs. >> (cbranch_bftst<mode>_insn, cstore_bftst<mode>_insn): New >> patterns. >> (movsi_m68k_movsi_m68k2, movsi_cf, unnamed movstrict patterns, >> unnamed movhi and movqi patterns, unnamed movsf, movdf and movxf >> patterns): Set attr "flags_valid". >> (truncsiqi2, trunchiqi2, truncsihi2): Remove manual CC_STATUS >> management. Set attr "flags_valid". >> (extendsidi2, extendplussidi, unnamed float_extendsfdf pattern, >> extendsfdf2_cf, fix_truncdfsi2, fix_truncdfhi2, fix_truncdfqi2, >> addi_sexthishl32, adddi_dilshr32, adddi_dilshr32_cf, >> addi_dishl32, subdi_sexthishl32, subdi_dishl32, subdi3): Remove >> manual CC_STATUS management. >> (addsi3_internal, addhi3, addqi3, subsi3, subhi3, subqi3, >> unnamed strict_lowpart subhi and subqi patterns): Set attr >> "flags_valid". >> (unnamed strict_lowpart addhi3 and addqi3 patterns): Likewise. >> Remove code to operate on address regs and assert the case >> does not occur. >> (unnamed mulsidi patterns, divmodhi4, udivmodhi4): Remove >> manual CC_STATUS_INIT. >> (andsi3_internal, andhi3, andqi3, iorsi3_internal, iorhi3, iorqi3, >> xorsi3_internal, xorhi3, xorqi3, negsi2_internal, >> negsi2_5200, neghi2, negqi2, one_cmplsi2_internal, one_cmplhi2, >> one_cmplqi2, unnamed strict_lowpart patterns >> for andhi, andqi, iorhi, iorqi, xorhi, xorqi, neghi, negqi, >> one_cmplhi and one_cmplqi): Set attr "flags_valid". >> (iorsi_zext_ashl16, iorsi_zext): Remove manual CC_STATUS_INIT. >> (ashldi_sexthi, ashlsi_16, ashlsi_17_24): Remove manual >> CC_STATUS_INIT. >> (ashlsi3, ashlhi3, ashlqi3, ashrsi3, ashrhi3, ashrqi3, lshrsi3, >> lshrhi3, shrqi3, rotlsi3, rotlhi3, rotlhi3_lowpart, rotlqi3, >> rotlqi3_lowpart, rotrsi3, rotrhi3, rotrhi_lowpart, rotrqi3, >> unnamed strict_low_part patterns for HI and >> QI versions): Set attr "flags_valid". >> (bsetmemqi, bsetmemqi_ext, bsetdreg, bchgdreg, bclrdreg, >> bclrmemqi, extzv_8_16_reg, extzv_bfextu_mem, insv_bfchg_mem, >> insv_bfclr_mem, insv_bfset_mem, extv_bfextu_reg, >> insv_bfclr_reg, insv_bfset_reg, dbne_hi, dbne_si, dbge_hi, >> dbge_si, extendsfxf2, extenddfxf2, ): Remove manual cc_status management. >> (various unnamed peepholes): Adjust compare/branch sequences >> for new cbranch patterns. >> (dbcc peepholes): Likewise, and output the comparison here >> as well. >> * config/m68k/predicates.md (valid_dbcc_comparison_p): Delete. >> (fp_src_operand): Allow constant zero. >> (address_reg_operand): New predicate. >> >> * rtl.h (inequality_comparisons_p): Remove declaration. >> * recog.h (next_insn_tests_no_inequality): Likewise. >> * rtlanal.c (inequality_comparisons_p): Delete function. >> * recog.c (next_insn_tests_no_inequality): Likewise. > My inclination is to ACK this as-is and let us iterate on any bugs that > pop up. Bernd has a long history with GCC and I absolutely trust his > ability, judgment and commitment to addressing any issues. > > While scanning this patch I did notice the introduction of > CC_STATUS_INIT in output_{and,ior,xor}si. You might want to check that. > > So unless there's objections over the next say 48-72 hrs, let's get the > kit in and we can iterate if there's further issues that need resolving. Any news on this? I would be in favor of merging these patches as I have tested them successfully on Debian by building the gcc-snapshot package with the patches applied. I used all four patches plus the additional one required to be able to build the kernel. I did not see the bootstrap problems that Mikael reported and Andreas has reported that the issues is not reliably reproducible on Aranym. He suspected that it might be a bug in the MMU emulation in Aranym which only triggers randomly depending on the current memory layout. I have used a patched gcc-10 to build the Debian kernel (package version 5.3.9-3) and it builds fine. The kernel also boots without any problems on my Amiga 4000 with 68060/50 accelerator and 128 MB RAM. So, I think we should get the patches merged so we have a chance to extensively test them in Debian with the help of the gcc-snapshot package that gets regularly updated in Debian. I will report any bugs that I am running into. Adrian
On 11/23/19 10:14 AM, John Paul Adrian Glaubitz wrote: > Hi Jeff! > >> On 11/13/19 6:23 AM, Bernd Schmidt wrote: >>> Once more with patch. >>> >>> >>> Bernd >>> >>> >>> m68k-2.diff >>> >>> PR target/91851 >>> * config/m68k/m68k-protos.h (output-dbcc_and_branch): Adjust >>> declaration. >>> (m68k_init_cc): New declaration. >>> (m68k_output_compare_di, m68k_output_compare_si, >>> m68k_output_compare_hi, m68k_output_compare_qi, >>> m68k_output_compare_fp, m68k_output_btst, m68k_output_bftst, >>> m68k_find_flags_value, m68k_output_scc, m68k_output_scc_float, >>> m68k_output_branch_integer, m68k_output_branch_integer_rev. >>> m68k_output_branch_float, m68k_output_branch_float_rev): >>> Likewise. >>> (valid_dbcc_comparison_p_2, flags_in_68881, >>> output_btst): Remove declaration. >>> * config/m68k/m68k.c (INCLDUE_STRING): Define. >>> (TARGET_ASM_FINAL_POSTSCAN_INSN): Define. >>> (valid_dbcc_comparison_p_2, flags_in_68881): Delete functions. >>> (flags_compare_op0, flags_compare_op1, flags_operand1, >>> flags_operand2, flags_valid): New static variables. >>> (m68k_find_flags_value, m68k_init_cc): New functions. >>> (handle_flags_for_move, m68k_asm_final_postscan_insn, >>> remember_compare_flags): New static functions. >>> (output_dbcc_and_branch): New argument CODE. Use it, and add >>> PLUS and MINUS to the possible codes. All callers changed. >>> (m68k_output_btst): Renamed from output_btst. Remove OPERANDS >>> and INSN arguments, add CODE arg. Return the comparison code >>> to use. All callers changed. Use CODE instead of >>> next_insn_tests_no_inequality, and replace cc_status management >>> with changing the return code. >>> (m68k_rtx_costs): Instead of testing for COMPARE, test for >>> RTX_COMPARE or RTX_COMM_COMPARE. >>> (output_move_simode, output_move_qimode): Call >>> handle_flags_for_move. >>> (notice_update_cc): Delete function. >>> (m68k_output_bftst, m68k_output_compare_di, m68k_output_compare_si, >>> m68k_output_compare_hi, m68k_output_compare_qi, >>> m68k_output_compare_fp, m68k_output_branch_integer, >>> m68k_output_branch_integer_rev, m68k_output_scc, >>> m68k_output_branch_float, m68k_output_branch_float_rev, >>> m68k_output_scc_float): New functions. >>> (output_andsi3, output_iorsi3, output_xorsi3): Call CC_STATUS_INIT >>> once at the start, and set flags_valid and flags_operand1 if the >>> flags are usable. >>> * config/m68k/m68k.h (CC_IN_68881, NOTICE_UPDATE_CC, >>> CC_OVERFLOW_UNUSABLE, CC_NO_CARRY, OUTPUT_JUMP): Remove >>> definitions. >>> (CC_STATUS_INIT): Define. >>> * config/m68k/m68k.md (flags_valid): New define_attr. >>> (tstdi, tstsi_internal_68020_cf, tstsi_internal, tsthi_internal, >>> tstqi_internal, tst<mode>_68881, tst<mode>_cf, cmpdi_internal, >>> cmpdi, unnamed cmpsi/cmphi/cmpqi patterns, cmpsi_cf, >>> cmp<mode>_68881, cmp<mode>_cf, unnamed btst patterns, >>> tst_bftst_reg, tst_bftst_reg, unnamed scc patterns, scc, >>> sls, sordered_1, sunordered_1, suneq_1, sunge_1, sungt_1, >>> sunle_1, sunlt_1, sltgt_1, fsogt_1, fsoge_1, fsolt_1, fsole_1, >>> bge0_di, blt0_di, beq, bne, bgt, bgtu, blt, bltu, bge, bgeu, >>> ble, bleu, bordered, bunordered, buneq, bunge, bungt, bunle, >>> bunlt, bltgt, beq_rev, bne_rev, bgt_rev, bgtu_rev, >>> blt_rev, bltu_rev, bge_rev, bgeu_rev, ble_rev, bleu_rev, >>> bordered_rev, bunordered_rev, buneq_rev, bunge_rv, bungt_rev, >>> bunle_rev, bunlt_rev, bltgt_rev, ctrapdi4, ctrapsi4, ctraphi4, >>> ctrapqi4, conditional_trap): Delete patterns. >>> (cbranchdi4_insn): New pattern. >>> (cbranchdi4): Don't generate cc0 patterns. When testing LT or GE, >>> test high part only. When testing EQ or NE, generate beq0_di >>> and bne0_di patterns directly. >>> (cstoredi4): When testing LT or GE, test high part only. >>> (both sets of cbranch<mode>4, cstore<mode>4): Don't generate cc0 >>> patterns. >>> (scc0_constraints, cmp1_constraints, cmp2_constraints, >>> scc0_cf_constraints, cmp1_cf_constraints, cmp2_cf_constraints, >>> cmp2_cf_predicate): New define_mode_attrs. >>> (cbranch<mode>4_insn, cbranch<mode>4_insn_rev, >>> cbranch<mode>4_insn_cf, cbranch<mode>4_insn_cf_rev, >>> cstore<mode>4_insn, cstore<mode>4_insn_cf for integer modes) >>> New patterns. >>> (cbranch<mode>4_insn_68881, cbranch<mode>4_insn_rev_68881): >>> (cbranch<mode>4_insn_cf, cbranch<mode>4_insn_rev_cf, >>> cstore<mode>4_insn_68881, cstore<mode>4_insn_cf for FP): >>> New patterns. >>> (cbranchsi4_btst_mem_insn, cbranchsi4_btst_reg_insn, >>> cbranchsi4_btst_mem_insn_1, cbranchsi4_btst_reg_insn_1): >>> Likewise. >>> (BTST): New define_mode_iterator. >>> (btst_predicate, btst_constraint, btst_range): New >>> define_mode_attrs. >>> (cbranch_bftst<mode>_insn, cstore_bftst<mode>_insn): New >>> patterns. >>> (movsi_m68k_movsi_m68k2, movsi_cf, unnamed movstrict patterns, >>> unnamed movhi and movqi patterns, unnamed movsf, movdf and movxf >>> patterns): Set attr "flags_valid". >>> (truncsiqi2, trunchiqi2, truncsihi2): Remove manual CC_STATUS >>> management. Set attr "flags_valid". >>> (extendsidi2, extendplussidi, unnamed float_extendsfdf pattern, >>> extendsfdf2_cf, fix_truncdfsi2, fix_truncdfhi2, fix_truncdfqi2, >>> addi_sexthishl32, adddi_dilshr32, adddi_dilshr32_cf, >>> addi_dishl32, subdi_sexthishl32, subdi_dishl32, subdi3): Remove >>> manual CC_STATUS management. >>> (addsi3_internal, addhi3, addqi3, subsi3, subhi3, subqi3, >>> unnamed strict_lowpart subhi and subqi patterns): Set attr >>> "flags_valid". >>> (unnamed strict_lowpart addhi3 and addqi3 patterns): Likewise. >>> Remove code to operate on address regs and assert the case >>> does not occur. >>> (unnamed mulsidi patterns, divmodhi4, udivmodhi4): Remove >>> manual CC_STATUS_INIT. >>> (andsi3_internal, andhi3, andqi3, iorsi3_internal, iorhi3, iorqi3, >>> xorsi3_internal, xorhi3, xorqi3, negsi2_internal, >>> negsi2_5200, neghi2, negqi2, one_cmplsi2_internal, one_cmplhi2, >>> one_cmplqi2, unnamed strict_lowpart patterns >>> for andhi, andqi, iorhi, iorqi, xorhi, xorqi, neghi, negqi, >>> one_cmplhi and one_cmplqi): Set attr "flags_valid". >>> (iorsi_zext_ashl16, iorsi_zext): Remove manual CC_STATUS_INIT. >>> (ashldi_sexthi, ashlsi_16, ashlsi_17_24): Remove manual >>> CC_STATUS_INIT. >>> (ashlsi3, ashlhi3, ashlqi3, ashrsi3, ashrhi3, ashrqi3, lshrsi3, >>> lshrhi3, shrqi3, rotlsi3, rotlhi3, rotlhi3_lowpart, rotlqi3, >>> rotlqi3_lowpart, rotrsi3, rotrhi3, rotrhi_lowpart, rotrqi3, >>> unnamed strict_low_part patterns for HI and >>> QI versions): Set attr "flags_valid". >>> (bsetmemqi, bsetmemqi_ext, bsetdreg, bchgdreg, bclrdreg, >>> bclrmemqi, extzv_8_16_reg, extzv_bfextu_mem, insv_bfchg_mem, >>> insv_bfclr_mem, insv_bfset_mem, extv_bfextu_reg, >>> insv_bfclr_reg, insv_bfset_reg, dbne_hi, dbne_si, dbge_hi, >>> dbge_si, extendsfxf2, extenddfxf2, ): Remove manual cc_status management. >>> (various unnamed peepholes): Adjust compare/branch sequences >>> for new cbranch patterns. >>> (dbcc peepholes): Likewise, and output the comparison here >>> as well. >>> * config/m68k/predicates.md (valid_dbcc_comparison_p): Delete. >>> (fp_src_operand): Allow constant zero. >>> (address_reg_operand): New predicate. >>> >>> * rtl.h (inequality_comparisons_p): Remove declaration. >>> * recog.h (next_insn_tests_no_inequality): Likewise. >>> * rtlanal.c (inequality_comparisons_p): Delete function. >>> * recog.c (next_insn_tests_no_inequality): Likewise. >> My inclination is to ACK this as-is and let us iterate on any bugs that >> pop up. Bernd has a long history with GCC and I absolutely trust his >> ability, judgment and commitment to addressing any issues. >> >> While scanning this patch I did notice the introduction of >> CC_STATUS_INIT in output_{and,ior,xor}si. You might want to check that. >> >> So unless there's objections over the next say 48-72 hrs, let's get the >> kit in and we can iterate if there's further issues that need resolving. > > Any news on this? I would be in favor of merging these patches as I have > tested them successfully on Debian by building the gcc-snapshot package > with the patches applied. I used all four patches plus the additional one > required to be able to build the kernel. Not really. I've already indicated to Bernd that he should go ahead and commit the changes and we can iterate on any problems that arise. > > I did not see the bootstrap problems that Mikael reported and Andreas has > reported that the issues is not reliably reproducible on Aranym. He suspected > that it might be a bug in the MMU emulation in Aranym which only triggers > randomly depending on the current memory layout. Good to know it wasn't reproducible and might ultimately be a bug in aranym. Jeff
On Sat, 2019-11-23 at 10:36 -0700, Jeff Law wrote: > > > Any news on this? I would be in favor of merging these patches as I > > have > > tested them successfully on Debian by building the gcc-snapshot > > package > > with the patches applied. I used all four patches plus the > > additional one > > required to be able to build the kernel. > > Not really. I've already indicated to Bernd that he should go ahead > and > commit the changes and we can iterate on any problems that arise. > > > > > I did not see the bootstrap problems that Mikael reported and > > Andreas has > > reported that the issues is not reliably reproducible on Aranym. He > > suspected > > that it might be a bug in the MMU emulation in Aranym which only > > triggers > > randomly depending on the current memory layout. > > Good to know it wasn't reproducible and might ultimately be a bug in > aranym. > Meanwhile, another patch that's supposed to do the same thing was posted: https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02131.html Cheers, Oleg
On 11/23/19 6:36 PM, Jeff Law wrote:
> Not really. I've already indicated to Bernd that he should go ahead and commit the changes and we can iterate on any problems that arise.
In the meantime I've made an aranym setup in addition to the qemu setup
I had, and I've not been able to reproduce failures like the ones
Andreas reported.
I'll spend a few more days trying to see if I can do something about the
bootstrap failure Mikael saw (currently trying to do a two-stage cross
build rather than a really slow bootstrap).
Bernd
On 11/23/19 9:53 PM, Bernd Schmidt wrote: > I'll spend a few more days trying to see if I can do something about the > bootstrap failure Mikael saw (currently trying to do a two-stage cross > build rather than a really slow bootstrap). Whew, I think I have it. One tst instruction eliminated when it shouldn't have been: move.w %a4,%d0 - tst.b %d0 - jeq .L352 + jeq .L353 And the reason - that's a movqi using move.w. The following should fix it. I'll run a few more tests, and then check it all in as Jeff suggested if things looks OK. Bernd
On 11/23/19 12:54 PM, Oleg Endo wrote: > On Sat, 2019-11-23 at 10:36 -0700, Jeff Law wrote: >> >>> Any news on this? I would be in favor of merging these patches as I >>> have >>> tested them successfully on Debian by building the gcc-snapshot >>> package >>> with the patches applied. I used all four patches plus the >>> additional one >>> required to be able to build the kernel. >> >> Not really. I've already indicated to Bernd that he should go ahead >> and >> commit the changes and we can iterate on any problems that arise. >> >>> >>> I did not see the bootstrap problems that Mikael reported and >>> Andreas has >>> reported that the issues is not reliably reproducible on Aranym. He >>> suspected >>> that it might be a bug in the MMU emulation in Aranym which only >>> triggers >>> randomly depending on the current memory layout. >> >> Good to know it wasn't reproducible and might ultimately be a bug in >> aranym. >> > > Meanwhile, another patch that's supposed to do the same thing was > posted: > > https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02131.html Yes. I'm aware. Given Bernd's was submitted first and looks generally sound, I think his has priority. jeff
On Nov 24 2019, Bernd Schmidt wrote: > Whew, I think I have it. One tst instruction eliminated when it > shouldn't have been: > > move.w %a4,%d0 > - tst.b %d0 > - jeq .L352 > + jeq .L353 > > And the reason - that's a movqi using move.w. The following should fix > it. Apparently that also fixed the testsuite regressions. Andreas.
On 11/25/19 12:26 PM, Andreas Schwab wrote: > On Nov 24 2019, Bernd Schmidt wrote: > >> Whew, I think I have it. One tst instruction eliminated when it >> shouldn't have been: >> >> move.w %a4,%d0 >> - tst.b %d0 >> - jeq .L352 >> + jeq .L353 >> >> And the reason - that's a movqi using move.w. The following should fix >> it. > > Apparently that also fixed the testsuite regressions. I still wish I knew how you managed to reproduce those. That would have been easier to debug than messing around with a three-stage cross-to-native setup. Bernd
On 11/23/19 6:36 PM, Jeff Law wrote: > Not really. I've already indicated to Bernd that he should go ahead and > commit the changes and we can iterate on any problems that arise. After the last fix, I did some more testing and since I feel confident that it really is in good shape now, I committed it. Thanks! Bernd
Hi Bernd! On 11/25/19 1:33 PM, Bernd Schmidt wrote: > On 11/23/19 6:36 PM, Jeff Law wrote: > >> Not really. I've already indicated to Bernd that he should go ahead and >> commit the changes and we can iterate on any problems that arise. > > After the last fix, I did some more testing and since I feel confident > that it really is in good shape now, I committed it. Thanks! Are all 4 + 2 patches in now? Thus, can we close the bug? Adrian
On 11/25/19 1:34 PM, John Paul Adrian Glaubitz wrote:
> Are all 4 + 2 patches in now? Thus, can we close the bug?
We're missing one piece for better autoinc generation, but that's a
small optimization issue. The cc0 conversion is complete.
Bernd
Hi Bernd, Thanks for the m68k work! Can you also update https://gcc.gnu.org/backends.html ? (Webseite repo ist now in git, cf. https://gcc.gnu.org/about.html#git ) Cheers, Tobias PS: I wonder whether some other archs also should be updated on that web page. On 11/25/19 1:33 PM, Bernd Schmidt wrote: > On 11/23/19 6:36 PM, Jeff Law wrote: > >> Not really. I've already indicated to Bernd that he should go ahead and >> commit the changes and we can iterate on any problems that arise. > After the last fix, I did some more testing and since I feel confident > that it really is in good shape now, I committed it. Thanks! > > > Bernd
On 11/25/19 1:38 PM, Bernd Schmidt wrote: > On 11/25/19 1:34 PM, John Paul Adrian Glaubitz wrote: >> Are all 4 + 2 patches in now? Thus, can we close the bug? > > We're missing one piece for better autoinc generation, but that's a > small optimization issue. The cc0 conversion is complete. Great. I have clicked "Accept" in Bountysource and hope the remaining backers will, too. Adrian
On 11/25/19 1:38 PM, Tobias Burnus wrote: > Thanks for the m68k work! Can you also update > https://gcc.gnu.org/backends.html ? Committed as obvious. Bernd
On Nov 25 2019, Bernd Schmidt wrote: > On 11/25/19 12:26 PM, Andreas Schwab wrote: >> On Nov 24 2019, Bernd Schmidt wrote: >> >>> Whew, I think I have it. One tst instruction eliminated when it >>> shouldn't have been: >>> >>> move.w %a4,%d0 >>> - tst.b %d0 >>> - jeq .L352 >>> + jeq .L353 >>> >>> And the reason - that's a movqi using move.w. The following should fix >>> it. >> >> Apparently that also fixed the testsuite regressions. > > I still wish I knew how you managed to reproduce those. It's undefined behaviour, so you can get all kinds of random results. The binary diff clearly shows the missing tstb insns. Andreas.
Hi! On Mon, Nov 25, 2019 at 01:38:53PM +0100, Tobias Burnus wrote: > Thanks for the m68k work! Can you also update > https://gcc.gnu.org/backends.html ? > PS: I wonder whether some other archs also should be updated on that web > page. Possibly. Probably? But, do you have any particular suggestions? Segher
On 11/25/19 3:40 PM, Segher Boessenkool wrote: > On Mon, Nov 25, 2019 at 01:38:53PM +0100, Tobias Burnus wrote: >> Thanks for the m68k work! Can you also update >> https://gcc.gnu.org/backends.html ? > >> PS: I wonder whether some other archs also should be updated on that web >> page. > > Possibly. Probably? > > But, do you have any particular suggestions? For m68k, it should be added that a free simulator is available as qemu has gained full emulation support for m68k as compared to the previous ColdFire-only emulation, i.e. the question mark in the "S" column should be removed. Same applies to i386, s390 and tilegx. These are all supported by qemu. Adrian
On Mon, 25 Nov 2019, Bernd Schmidt wrote: > On 11/23/19 6:36 PM, Jeff Law wrote: > > > Not really. I've already indicated to Bernd that he should go ahead and > > commit the changes and we can iterate on any problems that arise. > > After the last fix, I did some more testing and since I feel confident > that it really is in good shape now, I committed it. Thanks! I'm seeing a libgcc build failure for coldfire in my build-many-glibcs.py bot (m68k-linux-gnu configured --with-arch=cf --disable-multilib). That's building _mulsc3.o; I get assembler errors: /tmp/ccvO6ZQR.s: Assembler messages: /tmp/ccvO6ZQR.s:81: Error: invalid instruction for this architecture; needs M68K fpu (68020 [68k, 68ec020], 68030 [68ec030], 68040 [68ec040], 68060 [68ec060], cpu32 [68330, 68331, 68332, 68333, 68334, 68336, 68340, 68341, 68349, 68360]) -- statement `fcmp.x %fp0,%fp0' ignored /tmp/ccvO6ZQR.s:98: Error: invalid instruction for this architecture; needs M68K fpu (68020 [68k, 68ec020], 68030 [68ec030], 68040 [68ec040], 68060 [68ec060], cpu32 [68330, 68331, 68332, 68333, 68334, 68336, 68340, 68341, 68349, 68360]) -- statement `fcmp.x %fp1,%fp1' ignored I've not bisected, but r278642 was OK and r278694 had the failure, so I'm guessing it's probably this patch.
On 11/26/19 1:36 AM, Joseph Myers wrote: > I'm seeing a libgcc build failure for coldfire in my build-many-glibcs.py > bot (m68k-linux-gnu configured --with-arch=cf --disable-multilib). That's > building _mulsc3.o; I get assembler errors: I overlooked a difference in the 68881 vs coldfire patterns when I combined them. They use different suffixes for register compares (I only spotted the different constraints). The following seems to fix the assembler failures. I cannot properly test Coldfire however due to lack of hardware. Bernd
On Tue, 26 Nov 2019, Bernd Schmidt wrote: > On 11/26/19 1:36 AM, Joseph Myers wrote: > > I'm seeing a libgcc build failure for coldfire in my build-many-glibcs.py > > bot (m68k-linux-gnu configured --with-arch=cf --disable-multilib). That's > > building _mulsc3.o; I get assembler errors: > > I overlooked a difference in the 68881 vs coldfire patterns when I > combined them. They use different suffixes for register compares (I only > spotted the different constraints). > > The following seems to fix the assembler failures. I cannot properly > test Coldfire however due to lack of hardware. Thanks for fixing this (this is not a review of the patch). The soft-float ColdFire build (--with-arch=cf --with-cpu=54455 --disable-multilib) successfully built libgcc and glibc, but ran into an ICE building the glibc tests. Again, I've not bisected but this commit seems likely to be responsible. Compile the attached preprocessed source with -O2. during RTL pass: jump2 test-strncmp.c: In function 'test_main': test-strncmp.c:477:1: internal compiler error: in patch_jump_insn, at cfgrtl.c:1291 0x767a92 patch_jump_insn /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgrtl.c:1290 0x767b9f redirect_branch_edge /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgrtl.c:1317 0x76a722 rtl_redirect_edge_and_branch /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgrtl.c:1450 0x752e09 redirect_edge_and_branch(edge_def*, basic_block_def*) /scratch/jmyers/glibc/many10/src/gcc/gcc/cfghooks.c:373 0x122555c try_forward_edges /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgcleanup.c:562 0x122555c try_optimize_cfg /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgcleanup.c:2960 0x1226a8d cleanup_cfg(int) /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgcleanup.c:3174 0x1226cb8 execute /scratch/jmyers/glibc/many10/src/gcc/gcc/cfgcleanup.c:3354 Please submit a full bug report, with preprocessed source if appropriate. Please include the complete backtrace with any bug report. See <https://gcc.gnu.org/bugs/> for instructions.
On 11/26/19 3:21 AM, Joseph Myers wrote: > > The soft-float ColdFire build (--with-arch=cf --with-cpu=54455 > --disable-multilib) successfully built libgcc and glibc, but ran into an > ICE building the glibc tests. Again, I've not bisected but this commit > seems likely to be responsible. Compile the attached preprocessed source > with -O2. Try the following. This seems to be the same (preexisting) problem which got fixed for the normal 68k compare patterns, where an operand with nonimmediate_operand predicate allows constants in its constraints. Bernd
Hi Bernd! On 11/26/19 2:44 AM, Bernd Schmidt wrote: > I overlooked a difference in the 68881 vs coldfire patterns when I > combined them. They use different suffixes for register compares (I only > spotted the different constraints). > > The following seems to fix the assembler failures. I cannot properly > test Coldfire however due to lack of hardware. Angelo Dureghello (CC'ed) does active kernel development work on ColdFire and he might be able to help with testing on the platform. He also created two open-source ColdFire SBCs [1, 2]. I do have multiple ColdFire boards myself, but they are currently not set up and it would take me some time to get them running as I have never used them before. Adrian > [1] http://sysam.it/cff_amcore.html > [2] http://sysam.it/cff_stmark2.html
On Tue, 26 Nov 2019, Bernd Schmidt wrote: > On 11/26/19 3:21 AM, Joseph Myers wrote: > > > > The soft-float ColdFire build (--with-arch=cf --with-cpu=54455 > > --disable-multilib) successfully built libgcc and glibc, but ran into an > > ICE building the glibc tests. Again, I've not bisected but this commit > > seems likely to be responsible. Compile the attached preprocessed source > > with -O2. > > Try the following. This seems to be the same (preexisting) problem which > got fixed for the normal 68k compare patterns, where an operand with > nonimmediate_operand predicate allows constants in its constraints. I confirm that this and the previous patch together allow both the ColdFire configurations in build-many-glibcs.py to complete clean builds of GCC, glibc and glibc tests.
On Wed, Nov 27, 2019 at 12:18 AM Joseph Myers <joseph@codesourcery.com> wrote: > > On Tue, 26 Nov 2019, Bernd Schmidt wrote: > > > On 11/26/19 3:21 AM, Joseph Myers wrote: > > > > > > The soft-float ColdFire build (--with-arch=cf --with-cpu=54455 > > > --disable-multilib) successfully built libgcc and glibc, but ran into an > > > ICE building the glibc tests. Again, I've not bisected but this commit > > > seems likely to be responsible. Compile the attached preprocessed source > > > with -O2. > > > > Try the following. This seems to be the same (preexisting) problem which > > got fixed for the normal 68k compare patterns, where an operand with > > nonimmediate_operand predicate allows constants in its constraints. > > I confirm that this and the previous patch together allow both the > ColdFire configurations in build-many-glibcs.py to complete clean builds > of GCC, glibc and glibc tests. Both patches are OK. Thanks, Richard. > -- > Joseph S. Myers > joseph@codesourcery.com
Bernd Schmidt <bernds_cb1@t-online.de>: > On 11/23/19 9:53 PM, Bernd Schmidt wrote: > > I'll spend a few more days trying to see if I can do something > > about the bootstrap failure Mikael saw (currently trying to do a > > two-stage cross build rather than a really slow bootstrap). > > Whew, I think I have it. One tst instruction eliminated when it > shouldn't have been: > > move.w %a4,%d0 > - tst.b %d0 > - jeq .L352 > + jeq .L353 > > And the reason - that's a movqi using move.w. Can this problem also happen on older (pre-ccmode) GCC versions? Or was this only an issue of the ccmode conversion? What about the compare constraint errors? Are those also present on older GCC versions but never surfaced? Thanks, Gunther
On 11/28/19 8:53 PM, Gunther Nikl wrote:> Bernd Schmidt <bernds_cb1@t-online.de>: >> On 11/23/19 9:53 PM, Bernd Schmidt wrote: >> move.w %a4,%d0 >> - tst.b %d0 >> - jeq .L352 >> + jeq .L353 >> >> And the reason - that's a movqi using move.w. > > Can this problem also happen on older (pre-ccmode) GCC versions? Or was > this only an issue of the ccmode conversion? This was an error in the conversion (I think). > What about the compare constraint errors? Are those also present on > older GCC versions but never surfaced? Those were present, but presumably nothing ever tried to rerecognize a compare insn after reload (unlike jumps) so you wouldn't get a crash. I haven't checked whether it could have produced invalid assembly - I'm guessing probably not. Bernd
On Mon, Nov 25, 2019 at 03:44:44PM +0100, John Paul Adrian Glaubitz wrote: > On 11/25/19 3:40 PM, Segher Boessenkool wrote: > > On Mon, Nov 25, 2019 at 01:38:53PM +0100, Tobias Burnus wrote: > >> Thanks for the m68k work! Can you also update > >> https://gcc.gnu.org/backends.html ? > > > >> PS: I wonder whether some other archs also should be updated on that web > >> page. > > > > Possibly. Probably? > > > > But, do you have any particular suggestions? > > For m68k, it should be added that a free simulator is available as qemu > has gained full emulation support for m68k as compared to the previous > ColdFire-only emulation, i.e. the question mark in the "S" column should > be removed. > > Same applies to i386, s390 and tilegx. These are all supported by qemu. I've done this now (my first commit to the new git repo, wow that was easy to do :-) ) Thanks for the suggestion! Segher