Re: Don't mark operands of jump instructions available in gcse

[Daniel Berlin <dan at cgsoftware dot com>]

On Mon, 13 Aug 2001, Geoff Keating wrote:

> > Date: Mon, 13 Aug 2001 21:28:54 -0400 (EDT)
> > From: Daniel Berlin <dan@cgsoftware.com>
> > cc: <gcc-patches@gcc.gnu.org>
>
> > On Mon, 13 Aug 2001, Geoffrey Keating wrote:
> >
> > >
> > > This patch is being contributed mostly so it doesn't get lost.
> > > The original port was doing computation as part of a jump instruction,
> > > but it didn't work out because reload can't handle it.
> > >
> > > Anyway, gcse was also doing the wrong thing.  gcse should really
> > > use insert_insn_on_edge,
> > How was it doing hte wrong thing?
> > It does use insert_insn_on_edge.
> > Except in the abnormal edge case.
> > Though, it's using the wrong test.
> > It can't do it on abornmal critical edges.
> > simple abnormal edges should work just fine.
>
> The bit that caused the actual problem was in handle_avail_expr:
>
>   expr_set = single_set (insn_computes_expr);
>   if (!expr_set)
>     abort ();
>
> here insn_computes_expr was the JUMP_INSN, and of course couldn't
> be single_set.
>
But handle_avail_expr is only used in classic GCSE, which isn't edge
based anyway.

> Here is the place that should use insert_insn_on_edge:
>
>       /* Generate the new insn.  */
>       /* ??? If the change fails, we return 0, even though we created
> 	 an insn.  I think this is ok.  */
>       new_insn
> 	= emit_insn_after (gen_rtx_SET (VOIDmode, to,
> 					SET_DEST (expr_set)),
> 			   insn_computes_expr);
>
> this is of course wrong if 'insn_computes_expr' is a JUMP_INSN.
Sure.
>
> > > but I'm told all of gcse will go away
> > > sometime to be replaced with SSA-based code, so this just works
> > > around the problem by not trying to use expressions that
> > > are part of a branch.
> > This is wrong.
> > Saying "it will go away at some point" is not a good reason to use a
> > fix that just papers over the problem, at least, on the mainline.
> > There is nothing wrong with marking things in jump instructions. If there
> > is a problem, it's in insertion.
>
> Yes, this is correct.  We can't insert anything that is in a jump
> instruction.  So there's no point in marking it as available.

Wrong.
You are marking it not available in both PRE, and classic GCSE, when the
problem only exists in classic GCSE.

>
> I wouldn't consider this papering over a problem.  The code could
> never support this optimisation, it was not designed to do it.  New
> code could be written, but why bother when the right solution is to
> do a GCSE a completely different way?

Which is why edge based PRE does just this.
I have no problem with disabling the marking for classic GCSE, but you've
disabled it for PRE as well.

>
> --
> - Geoffrey Keating <geoffk@geoffk.org>
>