Re: 4 GCC regressions, 2 new, with your patch on 2002-07-18T09:56:17Z.

[Jan Hubicka <jh at suse dot cz>]

> > 
> > We do lose since often peepholes are asking for flags to do something
> > smart (like using xor for loading zero). Whenever we place unnecesary
> > code in between flags setter and user we lose.
> 
> The peephole code is supposed to look at the data flow, so it
> shouldn't matter how many instructions are between the setter and user.
> 
> > I think we simply should not extend liveness of hard registers when it
> > is easy to go without (ie we do not lose the optimization).
> 
> I don't think your patch represents a good design.  If it's important
> to keep hard register lifetimes short, then that problem should be
> solved in a general way, not by having special code for just one case
> in just one of the many routines that insert insns.
> 
> > > This code really doesn't feel right to me.  There could be many
> > > unexpected consequences.
> 
> For instance, you used single_set, but I think that's wrong in this
> case, because a CLOBBER might mean the instruction has a conflict with
> the insn you're moving over.
> 
> I think you also don't check that the basic block actually has more
> than one insn.  What happens if first instruction in a program is a
> (conditional?) jump?
Hi,
I am attaching the patch with the NULL insn bug fixed. Concerning the
other problems you mentioned I can't come with better sollution. To
summarize:
1) I've tracked down the ifcvt problem.  ifcvt should disable the
transformation but it does not.  So this patch is just about effectivity
of optimized code, not corectness now.
2) I still believe the patch is important for performance:
   a) currently number of optimizations will be disabled when the flags
      are live in a way. Ifcvt bug we are seeing is one of them, other
      problems are in combine that may want to add clobber of flags to
      complex instruction it creates, peepholers and loop optimizer
      doing similar trick to ifcvt.
   b) I would like to have some generic solution as you mentioned, but I
      unfortunately don't see how it can be done.  Flags are irritating
      special case for the compiler, as the compiler is generally built
      around assumption that there is unlimited supply of registers that
      simply does not hold for flags register that is just one on most
      machines and we do not register allocate it. Even worse it is
      killed by most instruction on i386.  If there were just IA-64 our
      life would be easier in this case.
   c) I hope to move the code motion to hoist* functions I've
      contributed that uses liveness information to validate motion to
      given point.  In case we will be systematically choosing points
      where flags are alive, we will disable all motion of instructions
      having clobbers in parallel
 3) The clobbers of the instruction I move over are not causing problems
    here as the instruction is known to have only one effect of storing
    newly created pseudo.

Hope it helps.  In short I agree it is ugly, but I can't think of better
way unfortunately and I believe it is critical for bringing some
benefits from code hoisting or doing basic block based GCSE as were
discussed earlier with Daniel.

Mon Jul 22 03:02:47 CEST 2002  Jan Hubicka  <jh@suse.cz>
	* gcse.c (insert_insn_end_bb): Do not insert in between flags setter
	and user.
Index: gcse.c
===================================================================
RCS file: /cvs/gcc/egcs/gcc/gcse.c,v
retrieving revision 1.211
diff -c -3 -p -r1.211 gcse.c
*** gcse.c	21 Jul 2002 19:38:08 -0000	1.211
--- gcse.c	22 Jul 2002 01:02:44 -0000
*************** insert_insn_end_bb (expr, bb, pre)
*** 4970,4975 ****
--- 4970,4977 ----
  #ifdef HAVE_cc0
        rtx note;
  #endif
+       rtx set_insn, set;
+ 
        /* It should always be the case that we can put these instructions
  	 anywhere in the basic block with performing PRE optimizations.
  	 Check this.  */
*************** insert_insn_end_bb (expr, bb, pre)
*** 5000,5005 ****
--- 5002,5016 ----
  	    insn = maybe_cc0_setter;
  	}
  #endif
+       /* We don't want to put instruction in between flag set and flag use
+          even when flags are represented as hard register.  */
+       set_insn = prev_nonnote_insn (insn);
+       if (set_insn && GET_CODE (set_insn) == INSN
+ 	  && (set = single_set (set_insn)) != 0
+ 	  && REG_P (SET_DEST (set))
+ 	  && REGNO (SET_DEST (set)) < FIRST_PSEUDO_REGISTER
+ 	  && !reg_overlap_mentioned_p (SET_DEST (set), pat))
+ 	insn = set_insn;
        /* FIXME: What if something in cc0/jump uses value set in new insn?  */
        new_insn = emit_insn_before (pat, insn);
      }