Created attachment 42924 [details] Test case "mips-buildroot-linux-gnu-gcc test.c -o test2 -Os -mbranch-cost=1" generates wrong code for me. This was seen with GCC 7.2 for MIPS32 r2 BE in OpenWrt / LEDE and with the free electrons toolchain. I also tested the GCC snapshot from 14. December 2017 and saw the same problem. This is the code: int mytest(mp_int * a, mp_digit b) { /* compare based on sign */ if (a->sign == 1) { return -1; } /* compare based on magnitude */ if (a->used > 1) { return 1; } /* compare the only digit of a to b */ if (a->dp[0] > b) { return 1; } else if (a->dp[0] < b) { return -1; } else { return 0; } } This is the wrong ASM: 004006c0 <mytest>: 4006c0: 8c830008 lw v1,8(a0) 4006c4: 24020001 li v0,1 4006c8: 1062000c beq v1,v0,4006fc <mytest+0x3c> 4006cc: 2402ffff li v0,-1 4006d0: 8c830000 lw v1,0(a0) 4006d4: 28630002 slti v1,v1,2 4006d8: 10600008 beqz v1,4006fc <mytest+0x3c> 4006dc: 00000000 nop 4006e0: 8c82000c lw v0,12(a0) 4006e4: 8c420000 lw v0,0(v0) 4006e8: 00a2182b sltu v1,a1,v0 4006ec: 14600005 bnez v1,400704 <mytest+0x44> 4006f0: 00000000 nop 4006f4: 0045102b sltu v0,v0,a1 4006f8: 00021023 negu v0,v0 4006fc: 03e00008 jr ra 400700: 00000000 nop 400704: 03e00008 jr ra 400708: 24020001 li v0,1 40070c: 00000000 nop In line 4006dc it should say "li v0,1" instead of nop. This code will return -1 if "if (a->used > 1) " is true, but it should return 1. I have also attached the code and compiled it with: ~/mips32--glibc--bleeding-edge/bin/mips-buildroot-linux-gnu-gcc test.c -o test2 -Os -mbranch-cost=1 When I compile it without "-mbranch-cost=1" it generates correct code. When I use this it also generates correct code: ~/mips32--glibc--bleeding-edge/bin/mips-buildroot-linux-gnu-gcc test.c -o test4 -Os -mbranch-cost=1 -funroll-loops
I only tested the GCC 7.X snapshot from 14. December 2017, not the GCC 8.X version.
Should I git bisect this or will someone else do this? If I should do that, is there some documentation on how to setup a test case and build and run this automatically?
Let me try to bisect that..
I did a small analysis. Can you please paste disassembly for the whole function (I want to see 4006fc). Thanks
The description already contains the full function incl. 4006fc. Do you need more?
(In reply to Hauke Mehrtens from comment #5) > The description already contains the full function incl. 4006fc. > Do you need more? Sorry, you are of course right. Let me try to write a reduction script.
git bisect between r235092 and r246594 show : It's a regression start by r240965. commit e55aa189d4c22c3e7992ebfbb7a90514647e219a Author: rts <rts@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Tue Oct 11 07:58:54 2016 +0000 [MIPS] Disable -mbranch-likely for -Os when targetting generic arch gcc/ * config/mips/mips-cpus.def: Replace PTF_AVOID_BRANCHLIKELY with PTF_AVOID_BRANCHLIKELY_ALWAYS for generic architecture and with PTF_AVOID_BRANCHLIKELY_SPEED for others. (mips2, mips3, mips4): Add PTF_AVOID_BRANCHLIKELY_SIZE to tune flags. * config/mips/mips.c (mips_option_override): Enable the branch likely depending on the tune flags and optimization level. * config/mips/mips.h (PTF_AVOID_BRANCHLIKELY): Remove. (PTF_AVOID_BRANCHLIKELY_SPEED): Define. (PTF_AVOID_BRANCHLIKELY_SIZE): Likewise. (PTF_AVOID_BRANCHLIKELY_ALWAYS): Likewise. git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@240965 138bc75d-0d04-0410-961f-82ee72b054a4 But the trunk GCC 8.X version is right. continue bisect which version fix this. Paul Hua.
On GCC 8.x, the r248351 fixed this. commit fd891ec7f659e8785c3ed5757f6e60c95117b837 Author: segher <segher@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Mon May 22 21:20:51 2017 +0000 cfgcleanup: Ignore clobbers in bb_is_just_return The function bb_is_just_return finds if the BB it is asked about does just a return and nothing else. It currently does not allow clobbers in the block either, which we of course can allow just fine. This patch changes that. * cfgcleanup.c (bb_is_just_return): Allow CLOBBERs. gcc/testsuite/ git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@248351 138bc75d-0d04-0410-961f-82ee72b054a4 Paul Hua
(In reply to Paul Hua from comment #8) > On GCC 8.x, the r248351 fixed this. > > commit fd891ec7f659e8785c3ed5757f6e60c95117b837 > Author: segher <segher@138bc75d-0d04-0410-961f-82ee72b054a4> > Date: Mon May 22 21:20:51 2017 +0000 > > cfgcleanup: Ignore clobbers in bb_is_just_return > > The function bb_is_just_return finds if the BB it is asked about does > just a return and nothing else. It currently does not allow clobbers > in the block either, which we of course can allow just fine. > > This patch changes that. > > > * cfgcleanup.c (bb_is_just_return): Allow CLOBBERs. > > gcc/testsuite/ > > > > git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@248351 > 138bc75d-0d04-0410-961f-82ee72b054a4 > > > Paul Hua GCC 8.0 Still reproducible: 0000000000000000 <mytest>: 0: 8c830008 lw v1,8(a0) 4: 24020001 li v0,1 8: 1062000d beq v1,v0,40 <mytest+0x40> c: 00000000 nop 10: 8c830000 lw v1,0(a0) 14: 28630002 slti v1,v1,2 18: 1060000b beqz v1,48 <mytest+0x48> 1c: 00000000 nop 20: dc820010 ld v0,16(a0) 24: dc420000 ld v0,0(v0) 28: 00a2182b sltu v1,a1,v0 2c: 14600006 bnez v1,48 <mytest+0x48> 30: 00000000 nop 34: 0045102b sltu v0,v0,a1 38: 03e00008 jr ra 3c: 0002102f dnegu v0,v0 40: 03e00008 jr ra 44: 2402ffff li v0,-1 48: 03e00008 jr ra 4c: 24020001 li v0,1 $ /opt/mips-gnu-git/bin/mips64-linux-gnu-gcc -v Using built-in specs. COLLECT_GCC=/opt/mips-gnu-git/bin/mips64-linux-gnu-gcc COLLECT_LTO_WRAPPER=/opt/mips-gnu-git/libexec/gcc/mips64-linux-gnu/8.0.1/lto-wrapper Target: mips64-linux-gnu Configured with: ../configure --target=mips64-linux-gnu --host=x86_64-redhat-linux-gnu --build=x86_64-redhat-linux-gnu --prefix=/opt/mips-gnu-git --disable-decimal-float --disable-dependency-tracking --disable-gold --disable-libgcj --disable-libgomp --disable-libmpx --disable-libquadmath --disable-libssp --disable-libunwind-exceptions --disable-shared --disable-silent-rules --disable-sjlj-exceptions --disable-threads --enable-__cxa_atexit --enable-checking=release --enable-gnu-unique-object --enable-initfini-array --enable-languages=c,c++ --enable-linker-build-id --enable-lto --enable-nls --enable-obsolete --enable-plugin --enable-targets=all --with-newlib --with-system-libunwind --with-system-zlib --with-arch=mips64r2 --with-abi=64 --with-arch_32=mips32r2 --with-fp-32=xx --enable-gnu-indirect-function Thread model: single gcc version 8.0.1 20180120 (experimental) (GCC) Regards, Leslie Zhai
(In reply to Leslie Zhai from comment #9) > GCC 8.0 Still reproducible: > > > 0000000000000000 <mytest>: > 0: 8c830008 lw v1,8(a0) > 4: 24020001 li v0,1 > 8: 1062000d beq v1,v0,40 <mytest+0x40> > c: 00000000 nop > 10: 8c830000 lw v1,0(a0) > 14: 28630002 slti v1,v1,2 > 18: 1060000b beqz v1,48 <mytest+0x48> > 1c: 00000000 nop > 20: dc820010 ld v0,16(a0) > 24: dc420000 ld v0,0(v0) > 28: 00a2182b sltu v1,a1,v0 > 2c: 14600006 bnez v1,48 <mytest+0x48> > 30: 00000000 nop > 34: 0045102b sltu v0,v0,a1 > 38: 03e00008 jr ra > 3c: 0002102f dnegu v0,v0 > 40: 03e00008 jr ra > 44: 2402ffff li v0,-1 > 48: 03e00008 jr ra > 4c: 24020001 li v0,1 Actually, I think that assembly looks unaffected by the issue. I have some doubts about the commits mentioned in this thread as well. r240965 looks like it simply changes the code generator in a way that makes the bug appear (without being the root cause of it) by affecting how and where branches are generated. It also looks like r248351 changes it again to avoid tripping over the bug, again without affecting the root cause. I don't know that much about GCC internals, so if I'm missing something here, please help me understand.
> > Actually, I think that assembly looks unaffected by the issue. > Right. > I have some doubts about the commits mentioned in this thread as well. > r240965 looks like it simply changes the code generator in a way that makes > the bug appear (without being the root cause of it) by affecting how and > where branches are generated. Yes, r240965 just to avoid usage of branch likely insns, not the root cause of this bug. so, I bisecting with -mno-branch-likely. (-fpreprocessed test.i -mel -quiet -dumpbase test.c -mbranch-cost=1 -mips32r2 -mabi=32 -mllsc -mno-shared -auxbase test -mno-branch-likely -Os -version -o test.s) Started with r235905. Author: segher <segher@138bc75d-0d04-0410-961f-82ee72b054a4> Date: Wed May 4 20:57:08 2016 +0000 shrink-wrap: Remove complicated simple_return manipulations Now that cfgcleanup knows how to optimize with return statements, the epilogue insertion code doesn't have to deal with it itself anymore. * function.c (emit_use_return_register_into_block): Delete. (gen_return_pattern): Delete. (emit_return_into_block): Delete. (active_insn_between): Delete. (convert_jumps_to_returns): Delete. (emit_return_for_exit): Delete. (thread_prologue_and_epilogue_insns): Delete all code dealing with simple_return for shrink-wrapped blocks. * shrink-wrap.c (try_shrink_wrapping): Insert simple_return at the end of blocks that need one. (get_unconverted_simple_return): Delete. (convert_to_simple_return): Delete. * shrink-wrap.c (get_unconverted_simple_return): Delete declaration. (convert_to_simple_return): Ditto.
It's the dbr ("reorg") pass that is deleting the insn.
There are *two* conditional branches leading to that "return 1". After dbr one is a delay-slot seq, and the other has lost the assignment of the return value.
Add Jeff in CC as maintainer of reorg (this bug is holding openwrt to gcc5).
(In reply to Felix Fietkau from comment #10) > (In reply to Leslie Zhai from comment #9) > > GCC 8.0 Still reproducible: > > > > > > 0000000000000000 <mytest>: > > 0: 8c830008 lw v1,8(a0) > > 4: 24020001 li v0,1 > > 8: 1062000d beq v1,v0,40 <mytest+0x40> > > c: 00000000 nop > > 10: 8c830000 lw v1,0(a0) > > 14: 28630002 slti v1,v1,2 > > 18: 1060000b beqz v1,48 <mytest+0x48> > > 1c: 00000000 nop > > 20: dc820010 ld v0,16(a0) > > 24: dc420000 ld v0,0(v0) > > 28: 00a2182b sltu v1,a1,v0 > > 2c: 14600006 bnez v1,48 <mytest+0x48> > > 30: 00000000 nop > > 34: 0045102b sltu v0,v0,a1 > > 38: 03e00008 jr ra > > 3c: 0002102f dnegu v0,v0 > > 40: 03e00008 jr ra > > 44: 2402ffff li v0,-1 > > 48: 03e00008 jr ra > > 4c: 24020001 li v0,1 > > Actually, I think that assembly looks unaffected by the issue. > Thanks for your response! I am also tracking the issue https://github.com/loongson-community/gcc/issues/4 I will compare with LLVM 7.x's Mips Target :)
Any update on this, or any way I could help in getting this fixed? It would be nice if we could finally switch OpenWrt to a more recent GCC version soon.
(In reply to Felix Fietkau from comment #16) > Any update on this, or any way I could help in getting this fixed? > It would be nice if we could finally switch OpenWrt to a more recent GCC > version soon. I would like to really help you. But it's really problematic to have a mips qemu machine with working network. Having that I would be able to run e.g. https://people.debian.org/~aurel32/qemu/mips/ and I would be able to debug that locally. Any help how to play with?
Marxin, you have a cfarm account and access to gcc22 / 23 / 24 which are mips64 machines. If you need to change ssh keys see here: https://cfarm.tetaneutral.net/login/
(In reply to Laurent GUERBY from comment #18) > Marxin, you have a cfarm account and access to gcc22 / 23 / 24 which are > mips64 machines. If you need to change ssh keys see here: > https://cfarm.tetaneutral.net/login/ Works for me, thank you.
But the machine is mips64: Linux erpro8-fsf2 4.1.4 #1 SMP PREEMPT Mon Aug 3 14:22:54 PDT 2015 mips64 GNU/Linux Is it a target where I can reproduce the issue? Which --target option do you use for a cross-compiler?
I'm not familiar with mips ABIs but on gcc mips machine there's 32 bit code. root@erpro8-fsf1:~# file /bin/ls /bin/ls: ELF 32-bit MSB executable, MIPS, MIPS-II version 1 (SYSV), dynamically linked (uses shared libs), for GNU/Linux 2.6.26, BuildID[sha1]=0x2b2fe81ce221a20ae62f4d516d1553fab61f9087, with unknown capability 0x41000000 = 0xf676e75, with unknown capability 0x10000 = 0x70401, stripped root@erpro8-fsf1:~# gcc -dumpspecs ... mabi=n32/mabi=32/mabi=64
Investigating.
Created attachment 43497 [details] Tentative fix To be tested.
Created attachment 43498 [details] Test case for internal compiler error (musl source file)´ When I test it with a patched gcc 7.3.x, I get this error on compiling remquo.c from musl with: -c -Os -pipe -mno-branch-likely -mips32r2 -mtune=24kc -fno-caller-saves -fno-plt -msoft-float remquo.c: In function 'remquo': remquo.c:82:1: internal compiler error: Segmentation fault } ^ 0x9dd06f crash_signal /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/toplev.c:337 0x9797f8 fix_reg_dead_note /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/reorg.c:1787 0x97e409 relax_delay_slots /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/reorg.c:3231 0x97e409 dbr_schedule /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/reorg.c:3723 0x97e409 rest_of_handle_delay_slots /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/reorg.c:3864 0x97e409 execute /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/reorg.c:3895 Line 1787 in reorg.c is this piece of code: if (REG_NOTE_KIND (link) != REG_DEAD || !REG_P (XEXP (link, 0))) continue;
> /var/nbd/lede/build_dir/toolchain-mipsel_24kc_gcc-7.3.0_musl/gcc-7.3.0/gcc/ > reorg.c:3895 > > Line 1787 in reorg.c is this piece of code: > if (REG_NOTE_KIND (link) != REG_DEAD > || !REG_P (XEXP (link, 0))) > continue; Weird, this looks like some ill-formed REG_DEAD note. I'll try to reproduce on SPARC tomorrow.
Created attachment 43499 [details] Second tentative fix Adjusted for second testcase but still to be tested.
On the original test case, it generates this code: 00400690 <mytest>: 400690: 8c830008 lw v1,8(a0) 400694: 24020001 li v0,1 400698: 10620011 beq v1,v0,4006e0 <mytest+0x50> 40069c: 00000000 nop 4006a0: 8c830000 lw v1,0(a0) 4006a4: 00000000 nop 4006a8: 28630002 slti v1,v1,2 4006ac: 1060000a beqz v1,4006d8 <mytest+0x48> 4006b0: 00000000 nop 4006b4: 8c82000c lw v0,12(a0) 4006b8: 00000000 nop 4006bc: 8c420000 lw v0,0(v0) 4006c0: 00000000 nop 4006c4: 00a2182b sltu v1,a1,v0 4006c8: 14600007 bnez v1,4006e8 <mytest+0x58> 4006cc: 00000000 nop 4006d0: 0045102b sltu v0,v0,a1 4006d4: 00021023 negu v0,v0 4006d8: 03e00008 jr ra 4006dc: 00000000 nop 4006e0: 03e00008 jr ra 4006e4: 2402ffff li v0,-1 4006e8: 03e00008 jr ra 4006ec: 24020001 li v0,1 It looks to me like it's generating lots of useless nop instructions after lw
Never mind, it seems that gcc 5.5 is doing that as well. I will run some more tests.
> Never mind, it seems that gcc 5.5 is doing that as hazard_nowell. I will run > some more tests. Yes, the nops are preexisting and counter-measures for pipeline hazards, but I don't know the MIPS port enough to say much more than that.
Just a note. I'm tracking a separate problem with delay slot filling that looks like it's related to handling of debug insns. I doubt it's the same problem, but if you stumble over it, be aware I'm testing a fix.
> Just a note. I'm tracking a separate problem with delay slot filling that > looks like it's related to handling of debug insns. I doubt it's the same > problem, but if you stumble over it, be aware I'm testing a fix. Is that PR debug/84545 or something related?
Created attachment 43510 [details] Final tentative fix I'm testing it on SPARC.
No. THe one I'm currently chasing is not 84545. I'm chasing a ton of ICEs due to debug insns appearing in places we didn't expect.
Author: ebotcazou Date: Mon Feb 26 16:29:30 2018 New Revision: 257996 URL: https://gcc.gnu.org/viewcvs?rev=257996&root=gcc&view=rev Log: PR rtl-optimization/83496 * reorg.c (steal_delay_list_from_target): Change REDUNDANT array from booleans to RTXes. Call fix_reg_dead_note on every non-null element. (steal_delay_list_from_fallthrough): Call fix_reg_dead_note on a redundant insn, if any. (relax_delay_slots): Likewise. (update_reg_unused_notes): Rename REDUNDANT_INSN to OTHER_INSN. Added: trunk/gcc/testsuite/gcc.c-torture/execute/20180226-1.c (with props) Modified: trunk/gcc/ChangeLog trunk/gcc/reorg.c trunk/gcc/testsuite/ChangeLog Propchange: trunk/gcc/testsuite/gcc.c-torture/execute/20180226-1.c ('svn:special' added)
Author: ebotcazou Date: Mon Feb 26 16:39:41 2018 New Revision: 258000 URL: https://gcc.gnu.org/viewcvs?rev=258000&root=gcc&view=rev Log: PR rtl-optimization/83496 * reorg.c (steal_delay_list_from_target): Change REDUNDANT array from booleans to RTXes. Call fix_reg_dead_note on every non-null element. (steal_delay_list_from_fallthrough): Call fix_reg_dead_note on a redundant insn, if any. (relax_delay_slots): Likewise. (update_reg_unused_notes): Rename REDUNDANT_INSN to OTHER_INSN. Added: branches/gcc-7-branch/gcc/testsuite/gcc.c-torture/execute/20180226-1.c - copied unchanged from r257998, trunk/gcc/testsuite/gcc.c-torture/execute/20180226-1.c Modified: branches/gcc-7-branch/gcc/ChangeLog branches/gcc-7-branch/gcc/reorg.c branches/gcc-7-branch/gcc/testsuite/ChangeLog
Fixed on mainline and 7 branch.
> Propchange: trunk/gcc/testsuite/gcc.c-torture/execute/20180226-1.c > ('svn:special' added) This is a symlink to /home/eric/build/gcc/mips-linux/pr83496.c which does not work on most people's machines ;-)
> This is a symlink to /home/eric/build/gcc/mips-linux/pr83496.c which > does not work on most people's machines ;-) It didn't really work on mine either after all so I quickly changed it. ;-)