| Message ID | 1451762204-13364-5-git-send-email-koriakin@0x04.net |
|---|---|
| State | New |
| Headers | show |
On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote: > When an unconditional jump with side effects targets an immediately > following label, rtl_tidy_fallthru_edge is called. Since it has side > effects, it doesn't remove the jump, but the label is still marked > as fallthru. This later causes a verification error. Do nothing in this > case instead. > > gcc/ChangeLog: > > * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps > with side effects. The change looks ok to me (although I'm not able to approve it). Could you please run regressions tests on x86_64 with that change? Perhaps a short comment in the code would be good. -Andreas-
On 21/01/16 11:05, Andreas Krebbel wrote: > On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote: >> When an unconditional jump with side effects targets an immediately >> following label, rtl_tidy_fallthru_edge is called. Since it has side >> effects, it doesn't remove the jump, but the label is still marked >> as fallthru. This later causes a verification error. Do nothing in this >> case instead. >> >> gcc/ChangeLog: >> >> * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps >> with side effects. > > The change looks ok to me (although I'm not able to approve it). Could you please run regressions > tests on x86_64 with that change? > > Perhaps a short comment in the code would be good. > > -Andreas- > OK, I'll run the testsuite and add a comment.
On 01/21/2016 03:05 AM, Andreas Krebbel wrote: > On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote: >> When an unconditional jump with side effects targets an immediately >> following label, rtl_tidy_fallthru_edge is called. Since it has side >> effects, it doesn't remove the jump, but the label is still marked >> as fallthru. This later causes a verification error. Do nothing in this >> case instead. >> >> gcc/ChangeLog: >> >> * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps >> with side effects. > > The change looks ok to me (although I'm not able to approve it). Could you please run regressions > tests on x86_64 with that change? > > Perhaps a short comment in the code would be good. I think the patch is technically fine, the question is does it fix a visible bug? I read the series as new feature enablement so I put this patch into my gcc7 queue. jeff
On 01/22/2016 12:10 AM, Jeff Law wrote: > On 01/21/2016 03:05 AM, Andreas Krebbel wrote: >> On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote: >>> When an unconditional jump with side effects targets an immediately >>> following label, rtl_tidy_fallthru_edge is called. Since it has side >>> effects, it doesn't remove the jump, but the label is still marked >>> as fallthru. This later causes a verification error. Do nothing in this >>> case instead. >>> >>> gcc/ChangeLog: >>> >>> * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps >>> with side effects. >> >> The change looks ok to me (although I'm not able to approve it). Could you please run regressions >> tests on x86_64 with that change? >> >> Perhaps a short comment in the code would be good. > I think the patch is technically fine, the question is does it fix a > visible bug? I read the series as new feature enablement so I put this > patch into my gcc7 queue. We need the patch for the S/390 split-stack implementation which we would like to see in GCC 6. I'm aware that this isn't stage 3 material but people seem to have reasons to really want split stack on S/390 asap and we would have to backport this feature anyway. Therefore I would prefer to have it in the official release already. That's the only common code change we would need for that. I've started a bootstrap and regression test for the patch also on Power. Do you see a chance we can get this into GCC 6? Bye, -Andreas-
On 22/01/16 08:44, Andreas Krebbel wrote: > On 01/22/2016 12:10 AM, Jeff Law wrote: >> On 01/21/2016 03:05 AM, Andreas Krebbel wrote: >>> On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote: >>>> When an unconditional jump with side effects targets an immediately >>>> following label, rtl_tidy_fallthru_edge is called. Since it has side >>>> effects, it doesn't remove the jump, but the label is still marked >>>> as fallthru. This later causes a verification error. Do nothing in this >>>> case instead. >>>> >>>> gcc/ChangeLog: >>>> >>>> * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps >>>> with side effects. >>> >>> The change looks ok to me (although I'm not able to approve it). Could you please run regressions >>> tests on x86_64 with that change? >>> >>> Perhaps a short comment in the code would be good. >> I think the patch is technically fine, the question is does it fix a >> visible bug? I read the series as new feature enablement so I put this >> patch into my gcc7 queue. > > We need the patch for the S/390 split-stack implementation which we would like to see in GCC 6. I'm > aware that this isn't stage 3 material but people seem to have reasons to really want split stack on > S/390 asap and we would have to backport this feature anyway. Therefore I would prefer to have it in > the official release already. That's the only common code change we would need for that. > > I've started a bootstrap and regression test for the patch also on Power. > > Do you see a chance we can get this into GCC 6? > > Bye, > > -Andreas- > I've tested the patch on x86_64, no regressions. I'm not entirely sure if the patch needs to go in for the current version of split-stack support. This patch fixed a showstopper bug on g5 CPUs when the patch still supported them. I haven't seen this bug with the z900 sequences (which are now the only ones left), but since we're still using unconditional jumps with side effects, I left it in just to be safe. The testsuite passes on s390x -fsplit-stack both with the patch and without it. So, I don't know. It seems to work now, probably because no optimization pass has a reason to touch that jump, but it may start to fail if someone adds a new optimization that tries to be smart with our prologue.
On 01/22/2016 12:44 AM, Andreas Krebbel wrote: > On 01/22/2016 12:10 AM, Jeff Law wrote: >> On 01/21/2016 03:05 AM, Andreas Krebbel wrote: >>> On 01/02/2016 08:16 PM, Marcin Kościelnicki wrote: >>>> When an unconditional jump with side effects targets an immediately >>>> following label, rtl_tidy_fallthru_edge is called. Since it has side >>>> effects, it doesn't remove the jump, but the label is still marked >>>> as fallthru. This later causes a verification error. Do nothing in this >>>> case instead. >>>> >>>> gcc/ChangeLog: >>>> >>>> * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps >>>> with side effects. >>> >>> The change looks ok to me (although I'm not able to approve it). Could you please run regressions >>> tests on x86_64 with that change? >>> >>> Perhaps a short comment in the code would be good. >> I think the patch is technically fine, the question is does it fix a >> visible bug? I read the series as new feature enablement so I put this >> patch into my gcc7 queue. > > We need the patch for the S/390 split-stack implementation which we would like to see in GCC 6. I'm > aware that this isn't stage 3 material but people seem to have reasons to really want split stack on > S/390 asap and we would have to backport this feature anyway. Therefore I would prefer to have it in > the official release already. That's the only common code change we would need for that. > > I've started a bootstrap and regression test for the patch also on Power. > > Do you see a chance we can get this into GCC 6? So I think it'd largely depend on what you do with the s390 specific bits -- if you decide to drop those in (ISTM that's your call), then I think adding the cfgrtl patch is probably the wise thing to do. So consider it approved for gcc-6 if/when you decide to go forward with the s390 specific bits. FWIW, the PA might run afoul of the code you're fixing as well. It's got add[i]b,tr and mov[i]b,tr which are unconditional jumps with other side effects. We never really used them all that much and once the PA8000 series came out, they were actually a performance lose, so they were disabled on the "modern" PA machines. Jeff
On 01/02/2016 12:16 PM, Marcin Kościelnicki wrote: > When an unconditional jump with side effects targets an immediately > following label, rtl_tidy_fallthru_edge is called. Since it has side > effects, it doesn't remove the jump, but the label is still marked > as fallthru. This later causes a verification error. Do nothing in this > case instead. > > gcc/ChangeLog: > > * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps > with side effects. OK for the trunk (gcc-7) It may not matter in practice, but you could try ripping out the other wide effects into individual insns and recognizing them. And if that works, then you can proceed to eliminate the jump, marking the fallthru label, etc. I think combine has some code to do similar things. jeff
diff --git a/gcc/ChangeLog b/gcc/ChangeLog index 56e31f6..4c7046f 100644 --- a/gcc/ChangeLog +++ b/gcc/ChangeLog @@ -1,5 +1,10 @@ 2016-01-02 Marcin Kościelnicki <koriakin@0x04.net> + * cfgrtl.c (rtl_tidy_fallthru_edge): Bail for unconditional jumps + with side effects. + +2016-01-02 Marcin Kościelnicki <koriakin@0x04.net> + * function.c (reposition_prologue_and_epilogue_notes): Avoid verification error if the last insn of prologue is an unconditional jump. diff --git a/gcc/cfgrtl.c b/gcc/cfgrtl.c index fbfc7cd..dc4c2b1 100644 --- a/gcc/cfgrtl.c +++ b/gcc/cfgrtl.c @@ -1762,6 +1762,8 @@ rtl_tidy_fallthru_edge (edge e) If block B consisted only of this single jump, turn it into a deleted note. */ q = BB_END (b); + if (JUMP_P (q) && !onlyjump_p (q)) + return; if (JUMP_P (q) && onlyjump_p (q) && (any_uncondjump_p (q)