That annoying dnrm2.f failure on i386

That annoying dnrm2.f failure on i386

[Bernd Schmidt]

I've spent some time trying to correct the dnrm2.f failure on the i386.
There are two problems in the compiler:
 - reg-stack.c can't handle computed gotos.  There was an attempt to fix
   this last year, but the code is nonsensical, and I've deleted it in the
   patch below.  Now, the compiler makes sure not to allocate stack regs
   across computed jumps to avoid the problem.
 - There was a bug stack_reg_life_analysis, which could cause incorrect
   data flow information to be generated.  If the loop that propagates flow
   information detected that it must restart, it did not do so immediately,
   and would corrupt the information if there was a subsequent iteration
   (because the variable "block" is changed to an essentially random value).

The fix for the second problem would also be appropriate for egcs-1.0.2, since
it's trivial (it only adds a break statement in the appropriate place) and may
also have an effect in code that does not use computed gotos.  I can send a
separate patch if necessary.
The patch adds a global variable, current_function_has_computed_jump, that
can be used if it turns out that other places need to be modified as well.

Unfortunately, dnrm2.f still fails. This appears to be due to the fact that
it reads uninitialized variables. Can someone who can actually parse Fortran
tell me how the test case should be fixed?
I've appended a test case in C that also shows the problem.

Bernd

-- test case

int foo1 = 0, foo2 = 0;
double b;

void bar ()
{
  void *p = &&l1;
  double a0 = 0, a1, a2, a3;

  if (foo1)
    p = &&l3;
  goto l2;
  l1:
  a0 += a1;
  goto out;

  l2:
  a1 = 5;

  l3:
  if (foo2)
    goto *p;

  l4:
  goto l1;

  out:
  b = a0;
}

int main ()
{
  bar ();
  if (b == 5)
    return 0;
  abort ();
}


-- patch

	* basic-block.h (basic_block_computed_jump_target): Declare.
	* flags.h: (current_function_has_computed_jump): Declare.
	* flow.c: (basic_block_computed_jump_target): Define.
	(flow_analysis): Allocate it. Set current_function_has_computed_jump
	to 0.
	(find_basic_blocks): Set current_function_has_computed_jump and
	elements of basic_block_computed_jump_target to 1 as appropriate.
	* function.c: (current_function_has_computed_jump): Define.
	* global.c (global_conflicts): Don't allocate pseudos into stack regs
	at the start of a block that is reachable by a computed jump.
	* reg-stack.c (stack_reg_life_analysis): If must restart, do so
	immediately.
	(subst_stack_regs): Undo change from Sep 4 1997.
	(uses_reg_or_mem): Now unused, deleted.
	* stupid.c (stupid_life_analysis): Compute
	current_function_has_computed_jump.
	(stupid_find_reg): Don't allocate stack regs if the function has a
	computed goto.

Index: basic-block.h
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/basic-block.h,v
retrieving revision 1.1.3.2
diff -c -p -r1.1.3.2 basic-block.h
*** basic-block.h	1998/02/05 13:09:40	1.1.3.2
--- basic-block.h	1998/02/17 15:22:06
*************** extern rtx *basic_block_head;
*** 108,113 ****
--- 108,118 ----
  
  extern rtx *basic_block_end;
  
+ /* Index by basic block number, determine whether the block can be reached
+    through a computed jump.  */
+ 
+ extern char *basic_block_computed_jump_target;
+ 
  /* Index by basic block number, get address of regset
     describing the registers live at the start of that block.  */
  
Index: flags.h
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/flags.h,v
retrieving revision 1.1.3.7
diff -c -p -r1.1.3.7 flags.h
*** flags.h	1998/02/16 19:03:56	1.1.3.7
--- flags.h	1998/02/17 18:17:19
*************** extern int current_function_has_nonlocal
*** 463,468 ****
--- 463,475 ----
  
  extern int current_function_has_nonlocal_goto;
  
+ /* If reg-stack will be run and this function has a computed goto, we can't
+    put pseudos into stack registers, since reg-stack will be unable to cope.
+    This variable is nonzero if we have found a computed goto.  This is
+    computed in flow.c or in stupid.c.  */
+ 
+ extern int current_function_has_computed_jump;
+ 
  /* Nonzero if GCC must add code to check memory access (used by Checker).  */
  
  extern int flag_check_memory_usage;
Index: flow.c
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/flow.c,v
retrieving revision 1.1.3.14
diff -c -p -r1.1.3.14 flow.c
*** flow.c	1998/02/16 19:03:57	1.1.3.14
--- flow.c	1998/02/17 17:11:22
*************** rtx *basic_block_head;
*** 197,202 ****
--- 197,207 ----
  
  rtx *basic_block_end;
  
+ /* Element N indicates whether basic block N can be reached through a
+    computed jump.  */
+ 
+ char *basic_block_computed_jump_target;
+ 
  /* Element N is a regset describing the registers live
     at the start of basic block N.
     This info lasts until we finish compiling the function.  */
*************** flow_analysis (f, nregs, file)
*** 359,364 ****
--- 364,370 ----
    n_basic_blocks = i;
    basic_block_head = (rtx *) oballoc (n_basic_blocks * sizeof (rtx));
    basic_block_end = (rtx *) oballoc (n_basic_blocks * sizeof (rtx));
+   basic_block_computed_jump_target = (char *) oballoc (n_basic_blocks);
    basic_block_drops_in = (char *) alloca (n_basic_blocks);
    basic_block_loop_depth = (short *) alloca (n_basic_blocks * sizeof (short));
    uid_block_number
*************** find_basic_blocks (f, nonlocal_label_lis
*** 410,416 ****
--- 416,424 ----
    block_live_static = block_live;
    bzero (block_live, n_basic_blocks);
    bzero (block_marked, n_basic_blocks);
+   bzero (basic_block_computed_jump_target, n_basic_blocks);
    bzero (active_eh_handler, (max_uid_for_flow + 1) * sizeof (rtx));
+   current_function_has_computed_jump = 0;
  
    /* Initialize with just block 0 reachable and no blocks marked.  */
    if (n_basic_blocks > 0)
*************** find_basic_blocks (f, nonlocal_label_lis
*** 618,633 ****
  			   and forced_labels list.  */
  			if (computed_jump_p (insn))
  			  {
  			    for (x = label_value_list; x; x = XEXP (x, 1))
! 			      mark_label_ref (gen_rtx_LABEL_REF (VOIDmode,
! 								 XEXP (x, 0)),
! 					      insn, 0);
  
  			    for (x = forced_labels; x; x = XEXP (x, 1))
! 			      mark_label_ref (gen_rtx_LABEL_REF (VOIDmode,
! 								 XEXP (x, 0)),
! 					      insn, 0);
! 			    }
  
  			/* If this is a CALL_INSN, then mark it as reaching
  			   the active EH handler for this CALL_INSN.  If
--- 626,650 ----
  			   and forced_labels list.  */
  			if (computed_jump_p (insn))
  			  {
+ 			    current_function_has_computed_jump = 1;
  			    for (x = label_value_list; x; x = XEXP (x, 1))
! 			      {
! 				int b = BLOCK_NUM (XEXP (x, 0));
! 				basic_block_computed_jump_target[b] = 1;
! 				mark_label_ref (gen_rtx_LABEL_REF (VOIDmode,
! 								   XEXP (x, 0)),
! 						insn, 0);
! 			      }
  
  			    for (x = forced_labels; x; x = XEXP (x, 1))
! 			      {
! 				int b = BLOCK_NUM (XEXP (x, 0));
! 				basic_block_computed_jump_target[b] = 1;
! 				mark_label_ref (gen_rtx_LABEL_REF (VOIDmode,
! 								   XEXP (x, 0)),
! 						insn, 0);
! 			      }
! 			  }
  
  			/* If this is a CALL_INSN, then mark it as reaching
  			   the active EH handler for this CALL_INSN.  If
Index: function.c
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/function.c,v
retrieving revision 1.1.3.9
diff -c -p -r1.1.3.9 function.c
*** function.c	1998/02/16 19:04:03	1.1.3.9
--- function.c	1998/02/17 17:08:51
*************** int current_function_has_nonlocal_label;
*** 126,131 ****
--- 126,138 ----
  
  int current_function_has_nonlocal_goto;
  
+ /* If reg-stack will be run and this function has a computed goto, we can't
+    put pseudos into stack registers, since reg-stack will be unable to cope.
+    This variable is nonzero if we have found a computed goto.  This is
+    computed in flow.c or in stupid.c.  */
+ 
+ int current_function_has_computed_jump;
+ 
  /* Nonzero if function being compiled contains nested functions.  */
  
  int current_function_contains_functions;
Index: global.c
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/global.c,v
retrieving revision 1.1.3.4
diff -c -p -r1.1.3.4 global.c
*** global.c	1998/02/16 19:04:22	1.1.3.4
--- global.c	1998/02/17 15:22:06
*************** global_conflicts ()
*** 675,680 ****
--- 675,689 ----
  	   allocno now live, and with each hard reg now live.  */
  
  	record_conflicts (block_start_allocnos, ax);
+ 
+ #ifdef STACK_REGS
+ 	/* Pseudos can't go in stack regs at the start of a basic block
+ 	   that can be reached through a computed goto, since reg-stack
+ 	   can't handle computed gotos.  */
+ 	if (basic_block_computed_jump_target[b])
+ 	  for (ax = FIRST_STACK_REG; ax <= LAST_STACK_REG; ax++)
+ 	    record_one_conflict (ax);
+ #endif
        }
  
        insn = basic_block_head[b];
Index: reg-stack.c
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/reg-stack.c,v
retrieving revision 1.1.3.7
diff -c -p -r1.1.3.7 reg-stack.c
*** reg-stack.c	1998/02/16 19:04:39	1.1.3.7
--- reg-stack.c	1998/02/17 18:45:54
*************** static void record_reg_life_pat		PROTO((
*** 249,255 ****
  static void get_asm_operand_lengths	PROTO((rtx, int, int *, int *));
  static void record_reg_life		PROTO((rtx, int, stack));
  static void find_blocks			PROTO((rtx));
- static int uses_reg_or_mem		PROTO((rtx));
  static rtx stack_result			PROTO((tree));
  static void stack_reg_life_analysis	PROTO((rtx, HARD_REG_SET *));
  static void replace_reg			PROTO((rtx *, int));
--- 249,254 ----
*************** find_blocks (first)
*** 1372,1409 ****
      }
  }
  
- /* Return 1 if X contain a REG or MEM that is not in the constant pool.  */
- 
- static int
- uses_reg_or_mem (x)
-      rtx x;
- {
-   enum rtx_code code = GET_CODE (x);
-   int i, j;
-   char *fmt;
- 
-   if (code == REG
-       || (code == MEM
- 	  && ! (GET_CODE (XEXP (x, 0)) == SYMBOL_REF
- 		&& CONSTANT_POOL_ADDRESS_P (XEXP (x, 0)))))
-     return 1;
- 
-   fmt = GET_RTX_FORMAT (code);
-   for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
-     {
-       if (fmt[i] == 'e'
- 	  && uses_reg_or_mem (XEXP (x, i)))
- 	return 1;
- 
-       if (fmt[i] == 'E')
- 	for (j = 0; j < XVECLEN (x, i); j++)
- 	  if (uses_reg_or_mem (XVECEXP (x, i, j)))
- 	    return 1;
-     }
- 
-   return 0;
- }
- 
  /* If current function returns its result in an fp stack register,
     return the REG.  Otherwise, return 0.  */
  
--- 1371,1376 ----
*************** stack_reg_life_analysis (first, stackent
*** 1552,1557 ****
--- 1519,1525 ----
  
  		  block = jump_block;
  		  must_restart = 1;
+ 		  break;
  
  		win:
  		  ;
*************** subst_stack_regs (insn, regstack)
*** 2701,2707 ****
  {
    register rtx *note_link, note;
    register int i;
-   rtx head, jump, pat, cipat;
    int n_operands;
  
    if (GET_CODE (insn) == CALL_INSN)
--- 2669,2674 ----
*************** subst_stack_regs (insn, regstack)
*** 2772,2810 ****
  
    if (GET_CODE (insn) == NOTE)
      return;
- 
-   /* If we are reached by a computed goto which sets this same stack register,
-      then pop this stack register, but maintain regstack. */
- 
-   pat = single_set (insn);
-   if (pat != 0
-       && INSN_UID (insn) <= max_uid
-       && GET_CODE (block_begin[BLOCK_NUM(insn)]) == CODE_LABEL
-       && GET_CODE (pat) == SET && STACK_REG_P (SET_DEST (pat)))
-     for (head = block_begin[BLOCK_NUM(insn)], jump = LABEL_REFS (head);
- 	 jump != head;
- 	 jump = LABEL_NEXTREF (jump))
-       {
- 	cipat = single_set (CONTAINING_INSN (jump));
- 	if (cipat != 0
- 	    && GET_CODE (cipat) == SET
- 	    && SET_DEST (cipat) == pc_rtx
- 	    && uses_reg_or_mem (SET_SRC (cipat))
- 	    && INSN_UID (CONTAINING_INSN (jump)) <= max_uid)
- 	  {
- 	    int from_block = BLOCK_NUM (CONTAINING_INSN (jump));
- 	    if (TEST_HARD_REG_BIT (block_out_reg_set[from_block],
- 				   REGNO (SET_DEST (pat))))
- 	      {
- 		struct stack_def old;
- 		bcopy (regstack->reg, old.reg, sizeof (old.reg));
- 		emit_pop_insn (insn, regstack, SET_DEST (pat), emit_insn_before);
- 		regstack->top += 1;
- 		bcopy (old.reg, regstack->reg, sizeof (old.reg));
- 		SET_HARD_REG_BIT (regstack->reg_set, REGNO (SET_DEST (pat)));
- 	      }
- 	  }
-       }
  
    /* If there is a REG_UNUSED note on a stack register on this insn,
       the indicated reg must be popped.  The REG_UNUSED note is removed,
--- 2739,2744 ----
Index: stupid.c
===================================================================
RCS file: /usr/local/cvs/gcc/gcc/stupid.c,v
retrieving revision 1.1.3.5
diff -c -p -r1.1.3.5 stupid.c
*** stupid.c	1998/02/16 19:04:56	1.1.3.5
--- stupid.c	1998/02/17 18:17:04
*************** stupid_life_analysis (f, nregs, file)
*** 134,139 ****
--- 134,141 ----
    register rtx last, insn;
    int max_uid, max_suid;
  
+   current_function_has_computed_jump = 0;
+ 
    bzero (regs_ever_live, sizeof regs_ever_live);
  
    regs_live = (char *) alloca (nregs);
*************** stupid_life_analysis (f, nregs, file)
*** 269,274 ****
--- 271,280 ----
  	     be live if it's also used to pass arguments.  */
  	  stupid_mark_refs (CALL_INSN_FUNCTION_USAGE (insn), insn);
  	}
+ #ifdef STACK_REGS
+       if (GET_CODE (insn) == JUMP_INSN && computed_jump_p (insn))
+ 	current_function_has_computed_jump = 1;
+ #endif
      }
  
    /* Now decide the order in which to allocate the pseudo registers.  */
*************** stupid_find_reg (call_preserved, class, 
*** 398,403 ****
--- 404,415 ----
    for (ins = born_insn; ins < dead_insn; ins++)
      IOR_HARD_REG_SET (used, after_insn_hard_regs[ins]);
  
+ #ifdef STACK_REGS
+   if (current_function_has_computed_jump)
+     for (i = FIRST_STACK_REG; i <= LAST_STACK_REG; i++)
+       SET_HARD_REG_BIT (used, i);
+ #endif
+   
    IOR_COMPL_HARD_REG_SET (used, reg_class_contents[(int) class]);
  
  #ifdef CLASS_CANNOT_CHANGE_SIZE

Re: That annoying dnrm2.f failure on i386

[Dave Love]

>>>>> "Bernd" == Bernd Schmidt <crux@pool.informatik.rwth-aachen.de> writes:

 Bernd> Unfortunately, dnrm2.f still fails. This appears to be due to
 Bernd> the fact that it reads uninitialized variables. 

Indeed.  It's certainly wrong.  I've checked in a fixed version as I
think I sent it in the first place.  Apologies if the lossage was
mine.

I haven't yet been able to test the fixed version with your patch.

 Bernd> Can someone who can actually parse Fortran tell me how the
 Bernd> test case should be fixed?

Actually, I can't parse that stuff, and this infamous bit of BLAS was
replaced some time ago, thank heavens, but just won't die...

Many thanks for the work!

--- dnrm2.f     1997/11/19 16:20:12     1.2
+++ dnrm2.f     1998/02/18 12:00:51     1.3
@@ -15,7 +15,7 @@ C     (This is actually an obsolete vers
 c     current Netlib BLAS.)

       integer i
-      double precision a(1:100)
+      double precision a(1:100), dnrm2
       do i=1,100
          a(i)=0.D0
       enddo
@@ -26,6 +26,8 @@ c     current Netlib BLAS.)
       integer i, incx, ix, j, n, next
       double precision   dx(1), cutlo, cuthi, hitest, sum, xmax,zero,one
       data   zero, one /0.0d0, 1.0d0/
+      data cutlo, cuthi / 8.232d-11,  1.304d19 /
+      j = 0
       if(n .gt. 0 .and. incx.gt.0) go to 10
          dnrm2  = zero

Re: That annoying dnrm2.f failure on i386

[Toon Moene]

[ ... patch for dnrm2.f failure on ix86's - hurray ! ... ]

>  Unfortunately, dnrm2.f still fails. This appears to be
>  due to the fact that it reads uninitialized variables.
>  Can someone who can actually parse Fortran tell me how
>  the test case should be fixed?

/usr/test/bin/g77 -Wuninitialized -O2 -S -v dnrm2.f
Reading specs from  
/usr/test/lib/gcc-lib/m68k-next-nextstep3/egcs-2.91.07/specs
gcc version egcs-2.91.07 980205 (gcc-2.8.0 release)
 /usr/test/lib/gcc-lib/m68k-next-nextstep3/egcs-2.91.07/f771  
dnrm2.f -fset-g77-defaults -quiet -dumpbase dnrm2.f -O2  
-Wuninitialized -version -fversion -o dnrm2.s
GNU F77 version egcs-2.91.07 980205 (gcc-2.8.0 release)  
(m68k-next-nextstep3) compiled by GNU C version egcs-2.91.07 980205  
(gcc-2.8.0 release).
GNU Fortran Front End version 0.5.22-19970929
dnrm2.f: In function `dnrm2':
dnrm2.f:3: warning: `xmax' might be used uninitialized in this function

One way of testing whether this is the real problem - short of  
implementing a full trap-on-uninitialised-variables option - is to  
initialise xmax as follows:

Add a line

      xmax = -1.0e38

right before the line

      if(n .gt. 0 .and. incx.gt.0) go to 10

i.e. the first `executable code'.

HTH,
Toon.

Re: That annoying dnrm2.f failure on i386

[Jeffrey A Law]

  > 	* basic-block.h (basic_block_computed_jump_target): Declare.
  > 	* flags.h: (current_function_has_computed_jump): Declare.
  > 	* flow.c: (basic_block_computed_jump_target): Define.
  > 	(flow_analysis): Allocate it. Set current_function_has_computed_jump
  > 	to 0.
  > 	(find_basic_blocks): Set current_function_has_computed_jump and
  > 	elements of basic_block_computed_jump_target to 1 as appropriate.
  > 	* function.c: (current_function_has_computed_jump): Define.
  > 	* global.c (global_conflicts): Don't allocate pseudos into stack regs
  > 	at the start of a block that is reachable by a computed jump.
  > 	* reg-stack.c (stack_reg_life_analysis): If must restart, do so
  > 	immediately.
  > 	(subst_stack_regs): Undo change from Sep 4 1997.
  > 	(uses_reg_or_mem): Now unused, deleted.
  > 	* stupid.c (stupid_life_analysis): Compute
  > 	current_function_has_computed_jump.
  > 	(stupid_find_reg): Don't allocate stack regs if the function has a
  > 	computed goto.
I've installed a slightly modified version of this patch.

In particular other passes can make use of this information
(haifa-sched.c & loop.c for example).  So I've make sure the
values are computed regardless of STACK_REGS and updated the
comments appropriately.

Thanks!

jeff

Re: That annoying dnrm2.f failure on i386

[Jeffrey A Law]

  In message < 9803312132.AA24000@moene.indiv.nluug.nl >you write:
  > [ ... Bernd Schmidt's patch elided ... ]
  > 
  > >  I've installed a slightly modified version of this patch.
  > >
  > >  In particular other passes can make use of this
  > >  information (haifa-sched.c & loop.c for example).  So
  > >  I've make sure the values are computed regardless of
  > >  STACK_REGS and updated the comments appropriately.
  > 
  > Great !  Now if you could install Richard Henderson's patch:
  > 
  > Thu Mar 12 09:53:14 1998  Richard Henderson  <rth@cygnus.com>
  > 
  > 	* com.c (ffecom_expr_): Revert Oct 22 change.  Instead take  
  > a WIDENP
  > 	argument so that we can respect the signedness of the  
  > original type.
  > 	(ffecom_init_0): Do sizetype initialization first.  Init new
  > 	ssizetype.
Well, this is really Richard's patch to take care of :-)  Odds are
it got lost in his queue somewhere...

Richard -- any reason not to install that patch?

jeff

Re: That annoying dnrm2.f failure on i386

[Richard Henderson]

On Wed, Apr 01, 1998 at 10:47:02PM -0700, Jeffrey A Law wrote:
> Well, this is really Richard's patch to take care of :-)  Odds are
> it got lost in his queue somewhere...

Indeed.  :-(

> Richard -- any reason not to install that patch?

I have a slightly different version of the patch for the development
sources, which adds `ssizetype' to stor-layout.c directly. 

Any objections to this?


r~

Re: That annoying dnrm2.f failure on i386

[Toon Moene]

[ ... Bernd Schmidt's patch elided ... ]

>  I've installed a slightly modified version of this patch.
>
>  In particular other passes can make use of this
>  information (haifa-sched.c & loop.c for example).  So
>  I've make sure the values are computed regardless of
>  STACK_REGS and updated the comments appropriately.

Great !  Now if you could install Richard Henderson's patch:

Thu Mar 12 09:53:14 1998  Richard Henderson  <rth@cygnus.com>

	* com.c (ffecom_expr_): Revert Oct 22 change.  Instead take  
a WIDENP
	argument so that we can respect the signedness of the  
original type.
	(ffecom_init_0): Do sizetype initialization first.  Init new
	ssizetype.

then all the current Fortran problems would be solved :-)

I have tried this patch on both an Alpha (solves the problem it was  
meant to solve and runs at least one other program correctly) and  
my m68k-next-nextstep3 system - just to check whether those changes  
wouldn't accidentally break a previously working combination.

(see http://egcs.cygnus.com/ml/egcs/1998-Mar/0456.html )

HTH,
Toon.

Re: That annoying dnrm2.f failure on i386

[Jim Wilson]

	I have a slightly different version of the patch for the development
	sources, which adds `ssizetype' to stor-layout.c directly. 

No objection from me.  We already have sbitsizetype and ubitsizetype, so it
makes sense to have ssizetype and usizetype also.

Jim

Re: That annoying dnrm2.f failure on i386

[Jeffrey A Law]

  In message < 19980401234156.49726@dot.cygnus.com >you write:
  > On Wed, Apr 01, 1998 at 10:47:02PM -0700, Jeffrey A Law wrote:
  > > Well, this is really Richard's patch to take care of :-)  Odds are
  > > it got lost in his queue somewhere...
  > 
  > Indeed.  :-(
  > 
  > > Richard -- any reason not to install that patch?
  > 
  > I have a slightly different version of the patch for the development
  > sources, which adds `ssizetype' to stor-layout.c directly. 
  > 
  > Any objections to this?
Not really, though obviously I haven't seen the patch :-)

jeff

Re: That annoying dnrm2.f failure on i386

[Richard Henderson]

On Thu, Apr 02, 1998 at 04:49:53PM -0800, Jim Wilson wrote:
> No objection from me.  We already have sbitsizetype and ubitsizetype, so it
> makes sense to have ssizetype and usizetype also.

Done.


r~