egcs 1.0.2 is broken on x86 and a patch for it.

egcs 1.0.2 is broken on x86 and a patch for it.

[H.J. Lu]

> 
> 
> Hello,
> 
> Richard - Could you please send the the correct patch for the fortran
> index problem from egcs 1.0.2 ? 
> 
> HJ and Richard: I have started to collect patches for 1.0.2. To date I
> have the libio patch and the optimization patch (to fix ImageMagick
> compile) from HJ. 
> 
> Other patches you might want in, I have to have them by the end of this
> week. Technically, by thursday, because I will need some time to rebuild
> the whole lot of packages...
> 

egcs 1.0.2 is broken on x86. The test case here is miscompiled by -O2.
Basically convert_regs () calls change_stack () with INSN to emit
some insn after INSN. But some insn has been added after INSN so
that change_stack () will emit some insn at the wrong place. As the
result, the floating point is broken on x86.

This patch seems to fix it. Could someone please take a look? Given
the bugs we have seen in egcs 1.0.2, I suggest egcs 1.0.3 be made.

Thanks.

H.J.
---
Wed Apr 15 08:01:56 1998  H.J. Lu  (hjl@gnu.org)

	* reg-stack.c (subst_asm_stack_regs): Change to return the last
	new insn generated by this function.
	(subst_stack_regs): Likewise.
	(convert_regs): Record the last newly generated insn and use
	it for change_stack () instead of INSN.

--- ../../../import/egcs/gcc/reg-stack.c	Wed Apr 15 07:30:26 1998
+++ ./reg-stack.c	Wed Apr 15 10:56:50 1998
@@ -262,9 +262,9 @@ static void move_for_stack_reg		PROTO((r
 static void swap_rtx_condition		PROTO((rtx));
 static void compare_for_stack_reg	PROTO((rtx, stack, rtx));
 static void subst_stack_regs_pat	PROTO((rtx, stack, rtx));
-static void subst_asm_stack_regs	PROTO((rtx, stack, rtx *, rtx **,
+static rtx subst_asm_stack_regs		PROTO((rtx, stack, rtx *, rtx **,
 					       char **, int, int));
-static void subst_stack_regs		PROTO((rtx, stack));
+static rtx subst_stack_regs		PROTO((rtx, stack));
 static void change_stack		PROTO((rtx, stack, stack, rtx (*) ()));
 
 static void goto_block_pat		PROTO((rtx, stack, rtx));
@@ -2407,9 +2407,11 @@ subst_stack_regs_pat (insn, regstack, pa
    stack-like regs in asm statements.  These rules are enforced by
    record_asm_stack_regs; see comments there for details.  Any
    asm_operands left in the RTL at this point may be assume to meet the
-   requirements, since record_asm_stack_regs removes any problem asm.  */
+   requirements, since record_asm_stack_regs removes any problem asm.
 
-static void
+   It returns the last new insn generated by this function. */
+
+static rtx
 subst_asm_stack_regs (insn, regstack, operands, operands_loc, constraints,
 		      n_inputs, n_outputs)
      rtx insn;
@@ -2688,14 +2690,18 @@ subst_asm_stack_regs (insn, regstack, op
 	      break;
 	    }
       }
+
+  return insn;
 }
 \f
 /* Substitute stack hard reg numbers for stack virtual registers in
    INSN.  Non-stack register numbers are not changed.  REGSTACK is the
    current stack content.  Insns may be emitted as needed to arrange the
-   stack for the 387 based on the contents of the insn.  */
+   stack for the 387 based on the contents of the insn.
 
-static void
+   It returns the last new insn generated by this function. */
+
+static rtx
 subst_stack_regs (insn, regstack)
      rtx insn;
      stack regstack;
@@ -2752,9 +2758,9 @@ subst_stack_regs (insn, regstack)
 	  decode_asm_operands (body, operands, operands_loc,
 			       constraints, NULL_PTR);
 	  get_asm_operand_lengths (body, n_operands, &n_inputs, &n_outputs);
-	  subst_asm_stack_regs (insn, regstack, operands, operands_loc,
-				constraints, n_inputs, n_outputs);
-	  return;
+	  return subst_asm_stack_regs (insn, regstack, operands,
+				       operands_loc, constraints,
+				       n_inputs, n_outputs);
 	}
 
       if (GET_CODE (PATTERN (insn)) == PARALLEL)
@@ -2772,7 +2778,7 @@ subst_stack_regs (insn, regstack)
      REG_UNUSED will already have been dealt with, so just return.  */
 
   if (GET_CODE (insn) == NOTE)
-    return;
+    return insn;
 
   /* If we are reached by a computed goto which sets this same stack register,
      then pop this stack register, but maintain regstack. */
@@ -2821,6 +2827,8 @@ subst_stack_regs (insn, regstack)
       }
     else
       note_link = &XEXP (note, 1);
+
+  return insn;
 }
 \f
 /* Change the organization of the stack so that it fits a new basic
@@ -3061,7 +3069,7 @@ static void
 convert_regs ()
 {
   register int block, reg;
-  register rtx insn, next;
+  register rtx insn, next, new;
   struct stack_def regstack;
 
   for (block = 0; block < blocks; block++)
@@ -3087,6 +3095,7 @@ convert_regs ()
       do
 	{
 	  insn = next;
+	  new = insn;
 	  next = NEXT_INSN (insn);
 
 	  /* Don't bother processing unless there is a stack reg
@@ -3094,7 +3103,8 @@ convert_regs ()
 	     floating point values).  */
 
 	  if (GET_MODE (insn) == QImode || GET_CODE (insn) == CALL_INSN)
-	    subst_stack_regs (insn, &regstack);
+	    /* We may have generated some new instructions here. */
+	    new = subst_stack_regs (insn, &regstack);
 
 	} while (insn != block_end[block]);
 
@@ -3117,7 +3127,7 @@ convert_regs ()
       /* Likewise handle the case where we fall into the next block.  */
 
       if ((block < blocks - 1) && block_drops_in[block+1])
-	change_stack (insn, &regstack, &block_stack_in[block+1],
+	change_stack (new, &regstack, &block_stack_in[block+1],
 		      emit_insn_after);
     }
 
--------
static __inline  double
mypow (double __x, double __y)
{
  register double __value;
  long __p = (long) __y;
  if (__y == (double) __p)
    {
      if (__p & 1)
	__x *= __x;
      if (__p == 0)
	return __x;
    }
  __asm __volatile__
    ("fmul %%st(1)"
     : "=t" (__value) :  "0" (__x), "u" (__y));
  return __value;
}

const double E1 = 2.71828182845904523536028747135;

double fact (double x)
{
  double corr;
  corr = 1.0;
  return corr * mypow(x/E1, x);
}

int main ()
{
  double y, z;

  y = fact (46.2);
  z = mypow (46.2/E1, 46.2);

  printf ("%26.19e, %26.19e\n", y, z);

  if (y > z)
    y -= z;
  else
    y = z - y;

  y /= z;
  if (y > 0.1)
    abort ();
 
  return 0;
}

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[Jeffrey A Law]

  In message < m0yPWgx-00058vC@ocean.lucon.org >you write:
  > egcs 1.0.2 is broken on x86.
I know folks have asked you before, so please use "has a bug" instead
of "is broken".  It seems silly, but using "is broken" makes the
situation sound a lot worse than it really is.


  The test case here is miscompiled by -O2.
Thanks.  I've added it to the testsuite.

  > Basically convert_regs () calls change_stack () with INSN to emit
  > some insn after INSN. But some insn has been added after INSN so
  > that change_stack () will emit some insn at the wrong place. As the
  > result, the floating point is broken on x86.
This is an extremely confusing explanation, primarily because you
use "insn"/INSN to refer to three different things.

Basically is sounds like you have

	insn 1
        insn 2


It sounds like we thought we wanted to insert after insn 1, but
because of other reg-stack actions we really wanted to insert
after insn 2.

  > This patch seems to fix it. Could someone please take a look? Given
  > the bugs we have seen in egcs 1.0.2, I suggest egcs 1.0.3 be made.
I must confess I don't undersatnd reg-stack all that well.  Assuming
your analysis and explanation are correct, then I think your change
is OK, though possibly incomplete.

In particular I worry that we need to pass "new" instead of "insn"
to the call to emit_pop_insn near the end of convert_regs.  I think
the call to goto_block_pat in convert_regs is OK.

I also worry that there may be cases were we need to insert after
insn 1 instead of after insn 2.  But I don't know reg-stack well
enough to know if that can actually happen.

You might ask Bernd Schmidt or Stan Cox to look at the change and
comment.

I'm going to go ahead and hesitantly install the change, but further
comments from folks that understand this code would be greatly
appreciated.

jeff

reg-stack.c problem

[H.J. Lu]

>   > Basically convert_regs () calls change_stack () with INSN to emit
>   > some insn after INSN. But some insn has been added after INSN so
>   > that change_stack () will emit some insn at the wrong place. As the
>   > result, the floating point is broken on x86.
> This is an extremely confusing explanation, primarily because you
> use "insn"/INSN to refer to three different things.
> 
> Basically is sounds like you have
> 
> 	insn 1
>         insn 2
> 
> 
> It sounds like we thought we wanted to insert after insn 1, but
> because of other reg-stack actions we really wanted to insert
> after insn 2.
> 
>   > This patch seems to fix it. Could someone please take a look? Given
>   > the bugs we have seen in egcs 1.0.2, I suggest egcs 1.0.3 be made.
> I must confess I don't undersatnd reg-stack all that well.  Assuming
> your analysis and explanation are correct, then I think your change
> is OK, though possibly incomplete.
> 
> In particular I worry that we need to pass "new" instead of "insn"
> to the call to emit_pop_insn near the end of convert_regs.  I think
> the call to goto_block_pat in convert_regs is OK.

You may be right.

> 
> I also worry that there may be cases were we need to insert after
> insn 1 instead of after insn 2.  But I don't know reg-stack well
> enough to know if that can actually happen.

I don't think that should happen.

> 
> You might ask Bernd Schmidt or Stan Cox to look at the change and
> comment.
> 
> I'm going to go ahead and hesitantly install the change, but further
> comments from folks that understand this code would be greatly
> appreciated.
> 

How does this patch against egcs 1.0 in CVS sound?

Thanks.

-- 
H.J. Lu (hjl@gnu.org)
--
Mon Apr 20 08:02:56 1998  H.J. Lu  (hjl@gnu.org)

	(convert_regs): Abort if the last newly created insn is a JUMP
	insn.
	Call goto_block_pat (), emit_pop_insn () and straighten_stack ()
	with NEW instead of INSN.

--- reg-stack.c.1	Sat Apr 18 18:23:12 1998
+++ reg-stack.c	Mon Apr 20 08:01:58 1998
@@ -3108,6 +3108,9 @@ convert_regs ()
 
 	} while (insn != block_end[block]);
 
+      if (new != insn && GET_CODE (new) == JUMP_INSN)
+	abort ();
+
       /* Something failed if the stack life doesn't match.  */
 
       GO_IF_HARD_REG_EQUAL (regstack.reg_set, block_out_reg_set[block], win);
@@ -3121,8 +3124,8 @@ convert_regs ()
 	 jump target if the target block's stack order hasn't been set
 	 yet.  */
 
-      if (GET_CODE (insn) == JUMP_INSN)
-	goto_block_pat (insn, &regstack, PATTERN (insn));
+      if (GET_CODE (new) == JUMP_INSN)
+	goto_block_pat (new, &regstack, PATTERN (insn));
 
       /* Likewise handle the case where we fall into the next block.  */
 
@@ -3151,11 +3154,11 @@ convert_regs ()
      for (reg = regstack.top; reg >= 0; reg--)
         if (regstack.reg[reg] < value_reg_low
 	    || regstack.reg[reg] > value_reg_high)
-           insn = emit_pop_insn (insn, &regstack,
+           insn = emit_pop_insn (new, &regstack,
 			    FP_MODE_REG (regstack.reg[reg], DFmode),
 			    emit_insn_after);
    }
-  straighten_stack (insn, &regstack);
+  straighten_stack (new, &regstack);
 }
 \f
 /* Check expression PAT, which is in INSN, for label references.  if

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[H.J. Lu]

> 
>   > Basically convert_regs () calls change_stack () with INSN to emit
>   > some insn after INSN. But some insn has been added after INSN so
>   > that change_stack () will emit some insn at the wrong place. As the
>   > result, the floating point is broken on x86.
> This is an extremely confusing explanation, primarily because you
> use "insn"/INSN to refer to three different things.
> 
> Basically is sounds like you have
> 
> 	insn 1
>         insn 2
> 
> 
> It sounds like we thought we wanted to insert after insn 1, but
> because of other reg-stack actions we really wanted to insert
> after insn 2.

It is close. Originally, we have

	insn 1

and we should insert something after insn 1. But in reg-stack.c,
something is added to fit the register stack and we get

	insn 1
	insn 2

before we are just about to insert something after insn 1.
Now we want to insert something after the original insn 1. Where
should we insert? After insn 1 or insn 2? insn 2 is added to
adjust the register stack and it is really the extention of
insn 1. If we add anything after insn 1 now, we just screw
up the purpose of adding insn 2 which is fit the register stack.
So things should be inserted after the original insn 1 should
be inserted after insn 2 now.

> 
>   > This patch seems to fix it. Could someone please take a look? Given
>   > the bugs we have seen in egcs 1.0.2, I suggest egcs 1.0.3 be made.
> I must confess I don't undersatnd reg-stack all that well.  Assuming
> your analysis and explanation are correct, then I think your change
> is OK, though possibly incomplete.
> 
> In particular I worry that we need to pass "new" instead of "insn"
> to the call to emit_pop_insn near the end of convert_regs.  I think
> the call to goto_block_pat in convert_regs is OK.
> 
> I also worry that there may be cases were we need to insert after
> insn 1 instead of after insn 2.  But I don't know reg-stack well
> enough to know if that can actually happen.
> 
> You might ask Bernd Schmidt or Stan Cox to look at the change and
> comment.
> 

Are they reading this list?

-- 
H.J. Lu (hjl@gnu.org)

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[H.J. Lu]

>   > Basically convert_regs () calls change_stack () with INSN to emit
>   > some insn after INSN. But some insn has been added after INSN so
>   > that change_stack () will emit some insn at the wrong place. As the
>   > result, the floating point is broken on x86.
> This is an extremely confusing explanation, primarily because you
> use "insn"/INSN to refer to three different things.
> 
> Basically is sounds like you have
> 
> 	insn 1
>         insn 2
> 
> 
> It sounds like we thought we wanted to insert after insn 1, but
> because of other reg-stack actions we really wanted to insert
> after insn 2.
> 
>   > This patch seems to fix it. Could someone please take a look? Given
>   > the bugs we have seen in egcs 1.0.2, I suggest egcs 1.0.3 be made.
> I must confess I don't undersatnd reg-stack all that well.  Assuming
> your analysis and explanation are correct, then I think your change
> is OK, though possibly incomplete.
> 
> In particular I worry that we need to pass "new" instead of "insn"
> to the call to emit_pop_insn near the end of convert_regs.  I think
> the call to goto_block_pat in convert_regs is OK.

I don't think using "new" will be a problem. At least, it should
introduce a new bug. I don't think subst_stack_regs will add a JUMP
INSN. If it does, that may be a bug in reg-stack.c by itself. We can
add

	if (new != insn && GET_CODE (new) == JUMP_INSN)
	  abort ();

to catch that.

How about straighten_stack () just before convert_regs () returns?

> 
> I also worry that there may be cases were we need to insert after
> insn 1 instead of after insn 2.  But I don't know reg-stack well

That is possible, but not likely. As I said in my previous email.
insn 2 is really the part of insn 1 in this case. Put anything
in between will defeat the whole purpose of inserting insn 2 after
insn 1.


-- 
H.J. Lu (hjl@gnu.org)

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[Bernd Schmidt]

> This is an extremely confusing explanation, primarily because you
> use "insn"/INSN to refer to three different things.
> 
> Basically is sounds like you have
> 
> 	insn 1
>         insn 2
> 
> 
> It sounds like we thought we wanted to insert after insn 1, but
> because of other reg-stack actions we really wanted to insert
> after insn 2.

> I'm going to go ahead and hesitantly install the change, but further
> comments from folks that understand this code would be greatly
> appreciated.

Here's how I see things.
In convert_regs, subst_stack_regs is called in a loop for each insn in
a basic block. subst_stack_regs makes sure that the insn is valid, by
substituting insns in it and by emitting additional insns around it.
It then updates the information in the regstack variable to match the
state of the stack _after all the insns it has emitted_. This means that
the loop in convert_regs handles things properly, since it takes NEXT_INSN
before calling subst_stack_regs, so it will use the proper insn in the
next iteration.
It then fails to recognize the case when subst_stack_regs had to emit
something after the last insn in the basic block. It will use the insn
that used to be the last one instead of one of those which subst_stack_regs
emitted after it, but this is inconsistent with the information in
reg_stack.

I think that all of the further calls in convert_regs need to use
PREV_INSN (next) instead of insn. This is essentially what HJ's two patches
achieve, although it can be done with a smaller patch (see below).
The question is what do to about this piece of code:

      if (GET_CODE (insn) == JUMP_INSN)
        goto_block_pat (insn, &regstack, PATTERN (insn));

subst_stack_regs will never emit a jump insn, so this test is not meaningful
for an instruction emitted by subst_stack_regs. I think that it is illegal
for subst_stack_regs to emit anything after a JUMP_INSN (just like you can't
have output reloads for a jump). So I think it's safe to abort if
  (GET_CODE (insn) == JUMP_INSN && insn != PREV_INSN (next))

The patch below, which applies against egcs-1.0.2, fixes HJ's test case and
does not introduce additional failures (in the egcs-1.0.2 test suite).
It should be almost equivalent to HJ's two patches applied on top of each
other, with the exception of the new sanity test (I'm testing whether INSN
is a JUMP, while he tests NEW, but since the only function in reg-stack
that can emit a jump is goto_block_pat, that can obviously never be true).
I hope that INSN can also never be true, that's why I abort in that case.

I'm just a bit surprised that this bug never showed up before. It can't
be that exotic to have a floating point instruction as the last one in
a basic block, can it?

Bernd

	* reg-stack.c (convert_regs): Make sure that the insn we use when
	processing the block exit is always the last one in the block.

*** ./reg-stack.c.orig-1	Wed Apr 22 10:15:16 1998
--- ./reg-stack.c	Wed Apr 22 10:19:49 1998
*************** convert_regs ()
*** 3097,3102 ****
--- 3097,3114 ----
  	    subst_stack_regs (insn, &regstack);
  
  	} while (insn != block_end[block]);
+       
+       /* For all further actions, INSN needs to be the last insn in
+          this basic block.  If subst_stack_regs inserted additional
+          instructions after INSN, it is no longer the last one at
+          this point.  */
+       next = PREV_INSN (next);
+ 
+       /* If subst_stack_regs inserted something after a JUMP_INSN, that
+          is almost certainly a bug.  */
+       if (GET_CODE (insn) == JUMP_INSN && insn != next)
+ 	abort ();
+       insn = next;
  
        /* Something failed if the stack life doesn't match.  */
  

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[H.J. Lu]

> 
> > This is an extremely confusing explanation, primarily because you
> > use "insn"/INSN to refer to three different things.
> > 
> > Basically is sounds like you have
> > 
> > 	insn 1
> >         insn 2
> > 
> > 
> > It sounds like we thought we wanted to insert after insn 1, but
> > because of other reg-stack actions we really wanted to insert
> > after insn 2.
> 
> > I'm going to go ahead and hesitantly install the change, but further
> > comments from folks that understand this code would be greatly
> > appreciated.
> 
> Here's how I see things.
> In convert_regs, subst_stack_regs is called in a loop for each insn in
> a basic block. subst_stack_regs makes sure that the insn is valid, by
> substituting insns in it and by emitting additional insns around it.
> It then updates the information in the regstack variable to match the
> state of the stack _after all the insns it has emitted_. This means that
> the loop in convert_regs handles things properly, since it takes NEXT_INSN
> before calling subst_stack_regs, so it will use the proper insn in the
> next iteration.
> It then fails to recognize the case when subst_stack_regs had to emit
> something after the last insn in the basic block. It will use the insn
> that used to be the last one instead of one of those which subst_stack_regs
> emitted after it, but this is inconsistent with the information in
> reg_stack.
> 
> I think that all of the further calls in convert_regs need to use
> PREV_INSN (next) instead of insn. This is essentially what HJ's two patches
> achieve, although it can be done with a smaller patch (see below).
> The question is what do to about this piece of code:
> 
>       if (GET_CODE (insn) == JUMP_INSN)
>         goto_block_pat (insn, &regstack, PATTERN (insn));
> 
> subst_stack_regs will never emit a jump insn, so this test is not meaningful
> for an instruction emitted by subst_stack_regs. I think that it is illegal
> for subst_stack_regs to emit anything after a JUMP_INSN (just like you can't
> have output reloads for a jump). So I think it's safe to abort if
>   (GET_CODE (insn) == JUMP_INSN && insn != PREV_INSN (next))
> 
> The patch below, which applies against egcs-1.0.2, fixes HJ's test case and
> does not introduce additional failures (in the egcs-1.0.2 test suite).
> It should be almost equivalent to HJ's two patches applied on top of each
> other, with the exception of the new sanity test (I'm testing whether INSN
> is a JUMP, while he tests NEW, but since the only function in reg-stack
> that can emit a jump is goto_block_pat, that can obviously never be true).
> I hope that INSN can also never be true, that's why I abort in that case.

I like your patch. Jeff, can we use Bernd's version?

> 
> I'm just a bit surprised that this bug never showed up before. It can't
> be that exotic to have a floating point instruction as the last one in
> a basic block, can it?
> 

We have seen/fixed 2 long standing x86 fp bugs so far :-(.

Thanks.


H.J.

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[Jim Wilson]

Yes, this patch looks good to me also.  Meanwhile, one of H.J. Lu's patches
has already been installed.  Should we remove H.J's patch (April 20) and
install patch instead?

Jim

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[Jeffrey A Law]

  In message < 199804240405.VAA28223@rtl.cygnus.com >you write:
  > Yes, this patch looks good to me also.  Meanwhile, one of H.J. Lu's patches
  > has already been installed.  Should we remove H.J's patch (April 20) and
  > install patch instead?
I suspect that's probably the way to go.  

jeff

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[H.J. Lu]

> 
>   In message < 199804240405.VAA28223@rtl.cygnus.com >you write:
>   > Yes, this patch looks good to me also.  Meanwhile, one of H.J. Lu's patches
>   > has already been installed.  Should we remove H.J's patch (April 20) and
>   > install patch instead?
> I suspect that's probably the way to go.  
> 
> jeff

I have said that I like that patch. I'd like to see that on in
egcs 1.0.3 instead.

-- 
H.J. Lu (hjl@gnu.org)

Re: egcs 1.0.2 is broken on x86 and a patch for it.

[Jim Wilson]

	Should we remove H.J's patch (April 20) and install patch instead?

I have done this.

Jim