From mboxrd@z Thu Jan 1 00:00:00 1970 From: hjl@lucon.org (H.J. Lu) To: crux@pool.informatik.rwth-aachen.de (Bernd Schmidt) Cc: law@cygnus.com, egcs@cygnus.com Subject: Re: egcs 1.0.2 is broken on x86 and a patch for it. Date: Wed, 22 Apr 1998 08:38:00 -0000 Message-id: References: X-SW-Source: 1998-04/msg00908.html > > > 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, ®stack, 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.