| Message ID | 21021.51193.481175.332729@pilspetsen.it.uu.se |
|---|---|
| State | New |
| Headers | show |
On 08/28/2013 03:50 AM, Mikael Pettersson wrote: > This patch fixes an ICE that occurs in #ifdef HAVE_cc0 code. The ICE > breaks both Java and Ada bootstrap on m68k-linux. There is also a > tiny C++ test case in the BZ entry. > > The ICE is triggered by the PR middle-end/50780 changes in r180192. > As explained in <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49847#c16>, > the effect of those changes is that an expression in EH context is > broken up so that the cc0 setter and user are put in separate BBs, which > normally isn't allowed. As fold_rtx sees the cc0 user, it calls > equiv_constant on prev_insn_cc0, but that is NULL due to the BB boundary, > resulting in the ICE. > > This patch checks if prev_insn_cc0 is NULL, and if so doesn't call > equiv_constant but sets const_arg to zero, which avoids the ICE and > makes fold_rtx leave the insn unchanged. > > Bootstrapped and regtested on m68k-linux, no regressions. This patch > has been in 4.6-based production compilers on m68k-linux since early 2012, > and in a 4.7-based compiler since early 2013. > > Ok for trunk and 4.8? > > [If approved I'll need help to commit it as I don't have commit rights.] > > gcc/ > > 2013-08-28 Mikael Pettersson <mikpe@it.uu.se> > > PR rtl-optimization/49847 > * cse.c (fold_rtx) <second case CC0>: If prev_insn_cc0 is NULL > don't call equiv_constant on it. I can't help but feel something different needs to be done here. It's always been the case that those two insns are expected to be contiguous and there's going to be code all over the place that makes that assumption and that model is what every GCC developer has expected for the last 20+ years. What precisely about Richi's patch causes the setter and user to end up in different blocks? I'm guessing we now consider the comparison itself as potentially trapping and thus we end the block immediately? The consumer then appears in the next block? I wonder if the comparison could be duplicated and marked as non-throwing at RTL generation time. Alternately (and probably better) would be to convert the remaining stragglers and drop the old cc0 support entirely. jeff
Jeff Law writes: > > * cse.c (fold_rtx) <second case CC0>: If prev_insn_cc0 is NULL > > don't call equiv_constant on it. > I can't help but feel something different needs to be done here. It's > always been the case that those two insns are expected to be contiguous > and there's going to be code all over the place that makes that > assumption and that model is what every GCC developer has expected for > the last 20+ years. > > What precisely about Richi's patch causes the setter and user to end up > in different blocks? I'm guessing we now consider the comparison itself > as potentially trapping and thus we end the block immediately? The > consumer then appears in the next block? Yes, r180192 makes tree-eh.c (stmt_could_throw_1_p) dive further into the expression, apparently finding a potentially trapping FP comparison, which causes the code generation difference. gcc-4.5 generated: .type _Z1ff, @function _Z1ff: .LFB0: .cfi_startproc link.w %fp,#0 .cfi_def_cfa 14, 8 ftst.s 8(%fp) fsge %d0 extb.l %d0 neg.l %d0 unlk %fp .cfi_offset 14, -8 rts .cfi_endproc while gcc >= 4.7 (with the ICE-preventing patch applied) generate: .type _Z1ff, @function _Z1ff: .LFB0: .cfi_startproc .cfi_personality 0,__gxx_personality_v0 .cfi_lsda 0,.LLSDA0 link.w %fp,#0 .cfi_offset 14, -8 .cfi_def_cfa 14, 8 .LEHB0: .LEHE0: .LEHB1: ftst.s 8(%fp) .LEHE1: fsge %d0 neg.b %d0 and.l #255,%d0 jra .L1 .L3: move.l %d0,-(%sp) jsr __cxa_begin_catch addq.l #4,%sp .LEHB2: jsr __cxa_end_catch .L1: .LEHE2: unlk %fp rts .cfi_endproc plus a lot of exception table data. (Note the label before the 'fsge'.) > Alternately (and probably better) > would be to convert the remaining stragglers and drop the old cc0 > support entirely. I'd love to see that happen, but it would take someone like me a lot of time to complete. (I did try an incremental conversion of just the FP stuff 2-3 years ago, but it didn't work well. Now I'd probably rewrite m68k.md and everything cc0-related in m68k.c from scratch.) /Mikael
On Wed, Aug 28, 2013 at 10:06 PM, Jeff Law <law@redhat.com> wrote: > On 08/28/2013 03:50 AM, Mikael Pettersson wrote: >> >> This patch fixes an ICE that occurs in #ifdef HAVE_cc0 code. The ICE >> breaks both Java and Ada bootstrap on m68k-linux. There is also a >> tiny C++ test case in the BZ entry. >> >> The ICE is triggered by the PR middle-end/50780 changes in r180192. >> As explained in <http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49847#c16>, >> the effect of those changes is that an expression in EH context is >> broken up so that the cc0 setter and user are put in separate BBs, which >> normally isn't allowed. As fold_rtx sees the cc0 user, it calls >> equiv_constant on prev_insn_cc0, but that is NULL due to the BB boundary, >> resulting in the ICE. >> >> This patch checks if prev_insn_cc0 is NULL, and if so doesn't call >> equiv_constant but sets const_arg to zero, which avoids the ICE and >> makes fold_rtx leave the insn unchanged. >> >> Bootstrapped and regtested on m68k-linux, no regressions. This patch >> has been in 4.6-based production compilers on m68k-linux since early 2012, >> and in a 4.7-based compiler since early 2013. >> >> Ok for trunk and 4.8? >> >> [If approved I'll need help to commit it as I don't have commit rights.] >> >> gcc/ >> >> 2013-08-28 Mikael Pettersson <mikpe@it.uu.se> >> >> PR rtl-optimization/49847 >> * cse.c (fold_rtx) <second case CC0>: If prev_insn_cc0 is NULL >> don't call equiv_constant on it. > > I can't help but feel something different needs to be done here. It's > always been the case that those two insns are expected to be contiguous and > there's going to be code all over the place that makes that assumption and > that model is what every GCC developer has expected for the last 20+ years. > > What precisely about Richi's patch causes the setter and user to end up in > different blocks? I'm guessing we now consider the comparison itself as > potentially trapping and thus we end the block immediately? The consumer > then appears in the next block? Btw, the handling for COND_EXPRs was simply adjusted to be the same as for if () conditions. From looking at the testcase, int f (float g) { try { return g >= 0; } catch (...) {} } can't you simply avoid expanding throwing stuff to a set-reg-based-on-compare instruction sequence? Or why does the issue not trigger on try { if (g >= 0) return 1; else 0; } catch (...) {} ? Richard. > I wonder if the comparison could be duplicated and marked as non-throwing at > RTL generation time. Alternately (and probably better) would be to convert > the remaining stragglers and drop the old cc0 support entirely. > > jeff > >
--- gcc-4.9-20130818/gcc/cse.c.~1~ 2013-08-05 22:16:05.000000000 +0200 +++ gcc-4.9-20130818/gcc/cse.c 2013-08-24 16:38:40.912803915 +0200 @@ -3194,9 +3194,14 @@ fold_rtx (rtx x, rtx insn) #ifdef HAVE_cc0 case CC0: - folded_arg = prev_insn_cc0; - mode_arg = prev_insn_cc0_mode; - const_arg = equiv_constant (folded_arg); + if (!prev_insn_cc0) + const_arg = 0; + else + { + folded_arg = prev_insn_cc0; + mode_arg = prev_insn_cc0_mode; + const_arg = equiv_constant (folded_arg); + } break; #endif