[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #10 from Jakub Jelinek jakub at gcc dot gnu.org --- The 4.8 version of the patch bootstrapped/regtested fine on both s390x-linux and s390-linux, both configured with --with-arch=z10 --with-tune=zEC12. Andreas, could you please bootstrap/regtest some 4.9 version of the patch?
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 Andreas Krebbel krebbel at gcc dot gnu.org changed: What|Removed |Added Attachment #30938|0 |1 is obsolete|| --- Comment #11 from Andreas Krebbel krebbel at gcc dot gnu.org --- Created attachment 30943 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30943action=edit Upstream patch I've tested this patch on s390 and s390x with --with-arch=z10 --with-tune=zEC12 and the default options on GCC head. No regressions.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #12 from Jakub Jelinek jakub at gcc dot gnu.org --- Thanks, are you going to post it to gcc-patches and commit then? Can I post the 4.8 patch there afterwards, or do you prefer some different alternative for 4.8?
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #13 from Andreas Krebbel krebbel at gcc dot gnu.org --- Author: krebbel Date: Tue Oct 1 13:33:02 2013 New Revision: 203060 URL: http://gcc.gnu.org/viewcvs?rev=203060root=gccview=rev Log: 2013-10-01 Jakub Jelinek ja...@redhat.com Andreas Krebbel andreas.kreb...@de.ibm.com PR target/58574 * config/s390/s390.c (s390_split_branches): Modify check for table jump insns. (s390_chunkify_start): Rearrange table jump insn check in order to deal with compare and branch insns correctly. 2013-10-01 Jakub Jelinek ja...@redhat.com PR target/58574 * gcc.c-torture/execute/pr58574.c: New testcase. Added: trunk/gcc/testsuite/gcc.c-torture/execute/pr58574.c Modified: trunk/gcc/ChangeLog trunk/gcc/config/s390/s390.c trunk/gcc/testsuite/ChangeLog
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #14 from Andreas Krebbel krebbel at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #12) Thanks, are you going to post it to gcc-patches and commit then? Can I post the 4.8 patch there afterwards, or do you prefer some different alternative for 4.8? Upstream version committed. Feel free to commit your version to 4.8 branch. Thanks!
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #15 from Jakub Jelinek jakub at gcc dot gnu.org --- Author: jakub Date: Tue Oct 1 13:50:30 2013 New Revision: 203062 URL: http://gcc.gnu.org/viewcvs?rev=203062root=gccview=rev Log: PR target/58574 * config/s390/s390.c (s390_chunkify_start): Handle tablejump_p first, continue when done, for other jumps look through PARALLEL unconditionally. * gcc.c-torture/execute/pr58574.c: New test. Added: branches/gcc-4_8-branch/gcc/testsuite/gcc.c-torture/execute/pr58574.c Modified: branches/gcc-4_8-branch/gcc/ChangeLog branches/gcc-4_8-branch/gcc/config/s390/s390.c branches/gcc-4_8-branch/gcc/testsuite/ChangeLog
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #2 from Jakub Jelinek jakub at gcc dot gnu.org --- I'd say the bug is in s390_chunkify_start: if (GET_CODE (pat) == PARALLEL XVECLEN (pat, 0) 2) pat = XVECEXP (pat, 0, 0); Dunno what exactly the 2 condition has been added for, JUMP_INSNs with PARALLEL with XVECLEN (pat, 0) == 2 are: 1) casesi_jump 2) *cmp_and_br_signed_mode, *cmp_and_br_unsigned_mode, *icmp_and_br_signed_mode, *icmp_and_br_unsigned_mode 3) *ccraw_to_int I can understand why you wouldn't want to handle 1) in there, and supposedly 3) should be split before machine reorg or worst case at the start of it. But 2), this case, it looks wrong not to do anything here. Note that similar condition is in s390_split_branches, no idea what do you want to do there. But perhaps TARGET_Z10 implies TARGET_ZARCH and thus s390_split_branches would be never called.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #3 from Jakub Jelinek jakub at gcc dot gnu.org --- Created attachment 30934 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30934action=edit Possible fix Possible fix.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #4 from Jakub Jelinek jakub at gcc dot gnu.org --- Created attachment 30935 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30935action=edit Alternate fix Another possibility. If the check is there really just to prevent handling tablejumps, I wonder why we can't do the tablejump handling first and only if it didn't do anything, handle all other jumps.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #5 from Andreas Krebbel krebbel at gcc dot gnu.org --- Thanks for tracking this down! (In reply to Jakub Jelinek from comment #2) I'd say the bug is in s390_chunkify_start: if (GET_CODE (pat) == PARALLEL XVECLEN (pat, 0) 2) pat = XVECEXP (pat, 0, 0); Dunno what exactly the 2 condition has been added for, JUMP_INSNs with The more complex loop jumps might have more than 2 I think. PARALLEL with XVECLEN (pat, 0) == 2 are: 1) casesi_jump 2) *cmp_and_br_signed_mode, *cmp_and_br_unsigned_mode, *icmp_and_br_signed_mode, *icmp_and_br_unsigned_mode 3) *ccraw_to_int I can understand why you wouldn't want to handle 1) in there, and supposedly 3) should be split before machine reorg or worst case at the start of it. But 2), this case, it looks wrong not to do anything here. Note that similar condition is in s390_split_branches, no idea what do you want to do there. But perhaps TARGET_Z10 implies TARGET_ZARCH and thus s390_split_branches would be never called. Exactly.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #6 from Andreas Krebbel krebbel at gcc dot gnu.org --- (In reply to Jakub Jelinek from comment #4) Created attachment 30935 [details] Alternate fix Another possibility. If the check is there really just to prevent handling tablejumps, I wonder why we can't do the tablejump handling first and only if it didn't do anything, handle all other jumps. I agree with that patch. Since tablejump_p is available it is easier to read the other way around. Thanks!
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #7 from Andreas Krebbel krebbel at gcc dot gnu.org --- Created attachment 30938 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30938action=edit Alternate fix - v2 Since tablejump_p is checking for JUMP_P anyway we could move the check even outside the jump insn check. I've also changed s390_split_branches to exit early for table jumps. Untested so far.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #8 from Jakub Jelinek jakub at gcc dot gnu.org --- Created attachment 30939 -- http://gcc.gnu.org/bugzilla/attachment.cgi?id=30939action=edit gcc48-pr58574.patch Yeah, perhaps. I'm attaching 4.8 version of the patch, which wasn't using tablejump_p at all. The patch also contains a testcase (which should be added to the 4.9 version of the patch too).
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 --- Comment #9 from Jakub Jelinek jakub at gcc dot gnu.org --- Note that I can't test this easily on the trunk, the partition I have access to has just 2 CPUs and 1GB of RAM, bootstrap/regtest would be very slow if it worked at all, but am testing the 4.8 version of the patch (via a build system scratch build).
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 Jakub Jelinek jakub at gcc dot gnu.org changed: What|Removed |Added Target Milestone|--- |4.8.2 Summary|[4.9 Regression] Wrong code |[4.8/4.9 Regression] Wrong |due to s390x machine reorg |code due to s390x machine |pass|reorg pass --- Comment #1 from Jakub Jelinek jakub at gcc dot gnu.org --- Reproduced with 4.8 branch too.
[Bug target/58574] [4.8/4.9 Regression] Wrong code due to s390x machine reorg pass
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=58574 Marek Polacek mpolacek at gcc dot gnu.org changed: What|Removed |Added Status|UNCONFIRMED |NEW Last reconfirmed||2013-09-29 CC||mpolacek at gcc dot gnu.org Ever confirmed|0 |1
