Bug 9786

State-Changed-From-To: open->analyzed
State-Changed-Why: Confirmed for 3.2.2, 3.3 and 3.4 branches. Used to work
    in 3.0, so is a regression.[

State-Changed-From-To: open-analyzed
State-Changed-Why: To compute 0.299*l, gcc generates this code to load l:

(insn 63 62 64 (nil) (set (reg:SF 74)
        (mem/s:SF (reg/v/f:SI 69 [ this ]) [13 <variable>.l+0 S4 A32])) -1 (nil)
    (nil))

(insn 64 63 65 (nil) (set (reg:DF 73)
        (float_extend:DF (reg:SF 74))) -1 (nil)
    (nil))

Both of these are deemed to possibly trap, since they involve floating
point math, so they go in separate basic blocks.  By .32.bbro, we have

(insn 63 216 167 2 0x402a2a24 (set (reg:SF 8 st(0) [orig:74 <variable>.l ] [74])
        (mem/s:SF (reg/v/u/f:SI 0 eax [orig:59 mi ] [59]) [13 <variable>.l+0 S4 A32])) 62 {*movsf_1} (insn_list 216 (nil))
    (expr_list:REG_DEAD (reg/v/u/f:SI 0 eax [orig:59 mi ] [59])
        (expr_list:REG_EH_REGION (const_int 2 [0x2])
            (nil))))
;; End of basic block 2, registers live:
 1 [dx] 6 [bp] 7 [sp] 8 [st] 16 [] 20 [frame]

;; Start of basic block 3, registers live: 1 [dx] 6 [bp] 7 [sp] 8 [st] 16 [] 20 [frame]
(note 167 63 64 3 [bb 3] NOTE_INSN_BASIC_BLOCK)

(insn 64 167 168 3 0x402a2a24 (set (reg:DF 8 st(0) [73])
        (float_extend:DF (reg:SF 8 st(0) [orig:74 <variable>.l ] [74]))) 93 {*extendsfdf2_1} (nil)
    (expr_list:REG_EH_REGION (const_int 2 [0x2])
        (nil)))
;; End of basic block 3, registers live:
 1 [dx] 6 [bp] 7 [sp] 8 [st] 16 [] 20 [frame]

Next, the reg-stack pass decides that insn 64 is a no-op and deletes it.
But that leaves bb3 as an empty basic block with an EH edge, which doesn't
make any sense, and fixup_abnormal_edges aborts.

Is it actually possible for a float_extend to trap?  If not, the simple fix
is to teach may_trap_p about that.  Otherwise, we need to delete the edge
somewhere along the way.

Responsible-Changed-From-To: unassigned->jason
Responsible-Changed-Why: i'll take a look

State-Changed-From-To: analyzed->open
State-Changed-Why: an->an

Responsible-Changed-From-To: jason->unassigned
Responsible-Changed-Why: rth says that extending a NaN can trap, so we can't just
    change may_trap_p.  So someone else will need to deal.

From: Nick Rasmussen <nick@ilm.com>
To: Cc: gcc-gnats@gcc.gnu.org, gcc-bugs@gcc.gnu.org
Subject: Re: optimization/9786: [3.2/3.3/3.4 regression] Ice in fixup_abnormal_edges
 with -fnon-call-exceptions -O2
Date: Tue, 01 Apr 2003 11:48:54 -0800

 I've already traced down what triggered the ICE:
 
 http://gcc.gnu.org/ml/gcc-bugs/2003-03/msg00025.html
 
 And got a reply from Jan Hubicka:
 
 http://gcc.gnu.org/ml/gcc-bugs/2003-03/msg00034.html
 
 It looks like it just didn't get attached to the gnats entry.
 
 -nick
 
 (resend - first reply got spamblocked by the list filters)
 
 Steven Bosscher wrote:
 > http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=9786 
 > 
 > 
 > Janis, this PR is a regression in 3.2, 3.3-to-be and mainline
 > from 3.0, but the breaking patch has not been identified yet.
 > Can you hunt it down?
 > 
 > For me this ICE triggers on i[56]86-pc-linux-gnu with:
 > 
 >    gcc-3.3 -c 9786.cc -fnon-call-exceptions -O -fforce-mem
 > 
 > Without -fforce-mem the ICE does not happen.
 > 
 > Greetz
 > Steven
 >

From: Steven Bosscher <s.bosscher@student.tudelft.nl>
To: gcc-gnats@gcc.gnu.org, gcc-bugs@gcc.gnu.org, nobody@gcc.gnu.org,
	nick@ilm.com, gcc-prs@gcc.gnu.org, janis187@us.ibm.com
Cc:  
Subject: Re: optimization/9786: [3.2/3.3/3.4 regression] Ice in fixup_abnormal_edges
 with -fnon-call-exceptions -O2
Date: Tue, 01 Apr 2003 20:30:42 +0200

 http://gcc.gnu.org/cgi-bin/gnatsweb.pl?cmd=view%20audit-trail&database=gcc&pr=9786
 
 Janis, this PR is a regression in 3.2, 3.3-to-be and mainline
 from 3.0, but the breaking patch has not been identified yet.
 Can you hunt it down?
 
 For me this ICE triggers on i[56]86-pc-linux-gnu with:
 
     gcc-3.3 -c 9786.cc -fnon-call-exceptions -O -fforce-mem
 
 Without -fforce-mem the ICE does not happen.
 
 Greetz
 Steven
 
 

For what it's worth, -mfpmath=sse -msse2 makes this go away for me when
compiling my application, as it uses the SSE instructions instead.  Of course
you don't get the 80-bit accumulators of the x87, and can only be used on later
CPU's with SSE instruction units.

One other thing.  I just submitted another bug, 11038, for a similar problem,
with a very very short test case (<10 lines).  Are these two related?

Still happens on the mainline (20030711).

A patch is available here for mainline:
http://gcc.gnu.org/ml/gcc-patches/2003-07/msg01772.html

I'm confused.

Olivier's message says:

> I first thought that deletion of trapping insns should not occur in the first
> place, but the insn is known to be a no-op there so this seems OK.

I'm not sure that this is OK.  In particular, the instruction shown is:

    (insn 21 19 87 2 (set (reg:DF 8 st(0) [63])
            (float_extend:DF (reg:SF 8 st(0)))) 136 {*extendsfdf2_1} (nil)
        (expr_list:REG_EH_REGION (const_int 1 [0x1])
            (nil)))

In an earlier piece of this audit trail, Jason says that Richard says that if
the value were a NaN this would trap.

Is the point that we do not end up actually generating an x86 instruction for
this operation because we realize that the source and destination register are
the same?  But wouldn't that be wrong, in that we're then throwing a potential
exception that was perhaps implied by the source program?

If it is OK to delete these instructions, then Olivier's patch looks OK to me,
except that the ??? comment should be updated.  If it is not OK to delete these
instructions, then we need to stop reg-stack from doing that.

Ideas?

Subject: Re:  [3.3/3.4 regression] Ice in fixup_abnormal_edges with -fnon-call-exceptions -O2


mmitchel at gcc dot gnu dot org wrote:
> I'm not sure that this is OK.  In particular, the instruction shown is:
> 
>     (insn 21 19 87 2 (set (reg:DF 8 st(0) [63])
>             (float_extend:DF (reg:SF 8 st(0)))) 136 {*extendsfdf2_1} (nil)
>         (expr_list:REG_EH_REGION (const_int 1 [0x1])
>             (nil)))
> 
> In an earlier piece of this audit trail, Jason says that Richard says that if
> the value were a NaN this would trap.
 
> Is the point that we do not end up actually generating an x86 instruction
> for this operation because we realize that the source and destination
> register are the same?

 In this case, yes, right after this comment in move_for_stack_reg:

      /* If this appears to be a no-op move, delete it, or else it
	 will confuse the machine description output patterns. 

      if (REGNO (src) == REGNO (dest))

 There are other cases in which some insn may be deleted though (just above
 this comment).

 However,

 - another comment, in the caller (subst_stack_regs), mentions again that
   deletions are performed for no-ops:

  /* subst_stack_regs_pat may have deleted a no-op insn.  If so, any
     REG_UNUSED will already have been dealt with, so just return.  */

 and

 - the file head comment provides a hint that deletions performed in this
   pass are safe because they relate to insns handling *virtual* stack
   registers:

   [...] After the hard register numbers are substituted, the semantics of an
   insn containing stack-like regs are not the same as for an insn with normal
   regs: for instance, it is not safe to delete an insn that appears to be a
   no-op move.

> But wouldn't that be wrong, in that we're then throwing a potential
> exception that was perhaps implied by the source program?

 I'm not sure, which is why I initially asked for advices and ended up with
 a note together with the patch submission.

 The audit trail mentions:

    rth says that extending a NaN can trap, so we can't just
    change may_trap_p. 
 
 It's not quite clear to me if there are conditions in which it couldn't
 trap, and I trusted the "no-op" qualifiers in the comments mentioned above
 in that respect.

 This code is pretty old, tough, so it is possible that the considerations
 we deal with today were not accounted for at that time.

> If it is OK to delete these instructions, then Olivier's patch looks OK to
> me, except that the ??? comment should be updated.  If it is not OK to
> delete these instructions, then we need to stop reg-stack from doing that.
 
 Agreed.

> Ideas?
 
 Does the additional input above ring a bell for anyone ?

 In any case, I think the comments will have to be updated to be more explicit
 on this issue.

 Thanks for your feedback.

 Olivier


 

Postponed until GCC 3.3.2.

Subject: Re:  [3.3/3.4 regression] Ice in fixup_abnormal_edges with -fnon-call-exceptions -O2


> > If it is OK to delete these instructions, then Olivier's patch looks OK to
> > me, except that the ??? comment should be updated.  If it is not OK to
> > delete these instructions, then we need to stop reg-stack from doing that.

 I thought a bit more about this, and am still unclear wether it is indeed
 appropriate for reg-stack to delete these insns.

 Since those deletions have always been there, could we consider fixing them
 by updating the edges too, and file another PR to investigate the possible
 (different) problem these deletions might cause with respect to exception 
 handling issues ?

 FWIW, merely removing the deletion leads to blowup at build time, which seems
 in accordance with what the associated comment says.

 Olivier


 

Trying to get a definitive assessment for Olivier's patch.

Olivier's patch applies cleanly (with a few offsets) to the 3.3 branch and cures
the regression.

Subject: Bug 9786

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	ebotcazou@gcc.gnu.org	2003-09-22 06:59:51

Modified files:
	gcc            : ChangeLog reg-stack.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/opt: reg-stack2.C 

Log message:
	PR target/9786
	* reg-stack.c (convert_regs_1): Purge possible dead eh edges
	after potential deletion of trapping insn. Avoids later ICE
	from call to fixup_abnormal_edges.
	(convert_regs_2): Stack the current block successors before
	processing this block, that is, before the potential deletion of
	dead edges by convert_regs_1, because these edges have been used
	to initialize the predecessors count.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.1116&r2=2.1117
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reg-stack.c.diff?cvsroot=gcc&r1=1.132&r2=1.133
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&r1=1.3075&r2=1.3076
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/reg-stack2.C.diff?cvsroot=gcc&r1=NONE&r2=1.1

Subject: Bug 9786

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_3-branch
Changes by:	ebotcazou@gcc.gnu.org	2003-09-22 07:11:27

Modified files:
	gcc            : ChangeLog reg-stack.c 
	gcc/testsuite  : ChangeLog 
Added files:
	gcc/testsuite/g++.dg/opt: reg-stack2.C 

Log message:
	PR target/9786
	* reg-stack.c (convert_regs_1): Purge possible dead eh edges
	after potential deletion of trapping insn. Avoids later ICE
	from call to fixup_abnormal_edges.
	(convert_regs_2): Stack the current block successors before
	processing this block, that is, before the potential deletion of
	dead edges by convert_regs_1, because these edges have been used
	to initialize the predecessors count.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.16114.2.749&r2=1.16114.2.750
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/reg-stack.c.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.117.2.3&r2=1.117.2.4
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=1.2261.2.285&r2=1.2261.2.286
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/testsuite/g++.dg/opt/reg-stack2.C.diff?cvsroot=gcc&only_with_tag=gcc-3_3-branch&r1=NONE&r2=1.1.2.1

I've applied Olivier's patch.