Patchwork [1/2] fix PR49847 ICE-on-HAVE_cc0 regression from PR50780 changes

login
register
mail settings
Submitter Mikael Pettersson
Date Aug. 28, 2013, 9:50 a.m.
Message ID <21021.51193.481175.332729@pilspetsen.it.uu.se>
Download mbox | patch
Permalink /patch/270423/
State New
Headers show

Comments

Mikael Pettersson - Aug. 28, 2013, 9:50 a.m.
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.
Jeff Law - Aug. 28, 2013, 8:06 p.m.
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
Mikael Pettersson - Aug. 29, 2013, 8:38 a.m.
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
Richard Guenther - Oct. 29, 2013, 3:12 p.m.
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
>
>

Patch

--- 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