From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bernd Schmidt To: Jeffrey A Law Cc: "H.J. Lu" , egcs@cygnus.com Subject: Re: egcs 1.0.2 is broken on x86 and a patch for it. Date: Wed, 22 Apr 1998 09:05:00 -0000 Message-id: References: <17624.893055503@hurl.cygnus.com> X-SW-Source: 1998-04/msg00910.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'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, ®stack); } 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. */