Re: PATCH: Re: ICE in 920624-1.c with -O3 -funroll-loops on

[law at redhat dot com]

In message <200202050510.g155AIxr021739@hiauly1.hia.nrc.ca>, "John David Anglin
" writes:
 > That's what my first patch did.  However, Honza pointed out that it
 > is valid for a case pattern to appear to use the jump table but in
 > fact do an indirect branch using a pointer.  If you kill the jump
 > table, then you need to kill the jumps first and they are difficult
 > to locate.  However, this does not apply to the VAX and I believe
 > the jump table can be safely deleted.
It seems to me that we do in fact kill the jumps first in cse.c and
cfgrtl.c, so I'm not sure I understand Honza's objection.

In fact, what I'm proposing is the set of actions to perform when we
transform any jump which uses a jump table into a simple jump -- regardless
of whether or not the jump is implemented as casesi, tablejump or a special
indirect jump.

The cases where my proposed rules can all be identified by the following code
(assuming INSN is the original jump):

  if ((tmp = JUMP_LABEL (insn)) != NULL_RTX
      && (tmp = NEXT_INSN (tmp)) != NULL_RTX
      && GET_CODE (tmp) == JUMP_INSN
      && (GET_CODE (PATTERN (tmp)) == ADDR_VEC
          || GET_CODE (PATTERN (tmp)) == ADDR_DIFF_VEC))


Furthermore, other insns don't reference the jump table -- they reference
the CODE_LABEL before the jump table.  That's the whole point behind leaving
the CODE_LABEL in the insn stream.  So I have a real hard time seeing why
it is unsafe to remove the jump table at the same time we convert its 
associated tablejump/casesi into a simple jump.

Maybe I'm just being dense.  


 > My current proposal essentially creates for the VAX the same situation
 > as we currently have on the PA (ie., the jump table remains around
 > until removed by dead code analysis).  Obviously, not as nice as
 > your suggestion below but it seems to work.
It'll break.  It's just a matter of getting into the right parts of
the compiler.  The most problematical areas are going to the hunks of code
that iterate over the insn stream without regards for basic block boundaries.

We've already run into cases where these unattached jump tables have caused
such code to core dump.  See:

  http://gcc.gnu.org/ml/gcc/2001-11/msg01355.html


While I marginally agree with the change that was installed -- it's safe, but
it needlessly caused the loop optimizer to give up on a loop -- all because
it had an unused jump table lying around in the middle of it.

It turns out that code was relatively easy to fix, but I have little
confidence that the rest of our compiler is ready to deal with a random,
unattached jump table, or with a jump table which references deleted
labels.

Before we switched to rely heavily on the cfg simplifier such cases were
caught and fixed by running the jump optimizer after any pass which might
have modified a jump.  One of the first things the jump optimizer did was
remove code between a BARRIER and the next CODE_LABEL.  That had the effect
of removing these unattached jump tables from the insn stream before the
other optimizers had the chance to trip over them.


 > The situations where a switch gets folded are relatively rare.
I agree.  However the amount of code that is prepared to deal with this
situation is rather vast -- particularly when you consider how we relied
upon the old jump optimizer to always clean up this turd before we moved
into the next optimization pass.

 > Loop
 > unrolling is where I first noted the problem.  Thus, unless you are
 > aware of other bugs affected by this problem, I would defer a better
 > fix until after 3.1 branches.
I ran into it just trying to bootstrap the compiler. :(

 > The current patch only affects the VAX because it is the only port
 > that defines CASE_DROPS_THROUGH, and thereby the only port where the
 > case insn is folded to a jump directed to the label before the jump table.
 > Thus, I don't believe patch will affect any other port.
True, but the underlying issue (unattached jump tables in the stream) affects
more than just the VAX.

 > > By having all 3 key items (tablejump, jump table and magic code label) in 
 > a
 > > single insn we avoid all the mess of knowing that magic items follow table
 > > jumps which must be kept in specific orders, and we avoid having random cr
 > ud
 > > existing between basic blocks.
 > 
 > Sounds wonderful.  In thinking about this, it became clear that separating
 > these components was just asking for trouble.
Agreed.  
jeff