Re: PATCH RFA: PR gcc/1532: jump to following instruction

[Ian Lance Taylor <ian at wasabisystems dot com>]

Richard Henderson <rth@redhat.com> writes:

> On Sun, Jan 25, 2004 at 09:39:45AM -0500, Ian Lance Taylor wrote:
> > 	PR gcc/1532
> > 	* reload1.c (reload): We can delete the CLOBBER of the return
> > 	register if it is followed by an instruction which modifies the
> > 	return register.
> 
> Hmm.  Certainly I believe this will produce correct code, and avoid
> the problems caused by unconditionally removing the clobber.  However,
> I'm thinking that it would be less hacky to have this done instead in
> flow.c, where we have nice things like real data flow available.
> 
> I'm thinking that all that may need changing is the following.
> 
> Index: flow.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/flow.c,v
> retrieving revision 1.578
> diff -c -p -d -r1.578 flow.c
> *** flow.c	6 Feb 2004 19:29:56 -0000	1.578
> --- flow.c	9 Feb 2004 23:44:55 -0000
> *************** insn_dead_p (struct propagate_block_info
> *** 2267,2280 ****
>       }
>   
>     /* A CLOBBER of a pseudo-register that is dead serves no purpose.  That
> !      is not necessarily true for hard registers.  */
> !   else if (code == CLOBBER && GET_CODE (XEXP (x, 0)) == REG
> ! 	   && REGNO (XEXP (x, 0)) >= FIRST_PSEUDO_REGISTER
> ! 	   && ! REGNO_REG_SET_P (pbi->reg_live, REGNO (XEXP (x, 0))))
> !     return 1;
>   
> -   /* We do not check other CLOBBER or USE here.  An insn consisting of just
> -      a CLOBBER or just a USE should not be deleted.  */
>     return 0;
>   }
>   
> --- 2267,2289 ----
>       }
>   
>     /* A CLOBBER of a pseudo-register that is dead serves no purpose.  That
> !      is not necessarily true for hard registers until after reload.  */
> !   else if (code == CLOBBER)
> !     {
> !       if (reload_completed)
> ! 	return 1;
> !       if (GET_CODE (XEXP (x, 0)) == REG
> ! 	  && REGNO (XEXP (x, 0)) >= FIRST_PSEUDO_REGISTER
> ! 	  && ! REGNO_REG_SET_P (pbi->reg_live, REGNO (XEXP (x, 0))))
> ! 	return 1;
> !     }
> ! 
> !   /* ??? A base USE is a historical relic.  It ought not be needed anymore.
> !      Instances where it is still used are either (1) temporary and the USE
> !      escaped the pass, (2) cruft and the USE need not be emitted anymore,
> !      or (3) hiding bugs elsewhere that are not properly representing data
> !      flow.  */
>   
>     return 0;
>   }
>   

I follow your reasoning, and indeed my original patch was more along
these lines, albeit still in reload rather than in flow.  However, I
got crashes in consistency checks, so I moved back to the more
conservative patch.

Your patch generates testsuite crashes just as my original patch did.
For example, this trivial function:
    int foo () { int i = 0; }
crashes at -O2 with
    foo1.c:1: internal compiler error: in verify_local_live_at_start, at flow.c:568

This is a sanity check verifying that after reload the registers live
at the start of a block do not change.  This is no longer the case
after your patch, because there is still a USE of the return register
at the end of the function.  Removing the CLOBBER means that the
return register is now live at the start of the function, which was
not previously the case.

At least in this particular case, the crash comes via this call from
schedule_insns():

  /* Don't update reg info after reload, since that affects
     regs_ever_live, which should not change after reload.  */
  update_life_info (blocks, UPDATE_LIFE_LOCAL,
		    (reload_completed ? PROP_DEATH_NOTES
		     : PROP_DEATH_NOTES | PROP_REG_INFO));

so there is already code worrying about this type of thing.

Another type of crash I see is this:
    double foo () { int i = 0; }
at -O, I get
    foo1.c:1: internal compiler error: in subst_stack_regs_pat, at reg-stack.c:1403

This is another sanity check picking up on the USE of the return
register for a function with a floating point return rtype.  The code
in reg-stack has some special handling for the CLOBBER which, without
your patch, appears at the end of a function which fails to return a
value.  The reg-stack code loads up a NaN in the CLOBBER, and marks
the register in-use.  Without that NaN and without marking the
register in USE, the reg-stack code sees a USE of a register which is
not, in fact, used, and it aborts.

The reg-stack case is interesting because without the CLOBBER we will
no longer generate the NaN which we previously generated.  That sounds
OK to me--a function that fails to return a value can return garbage,
and gcc can issue a warning for such a function anyhow--but it needs
to be pointed out.

There is another sanity check crash at sched-rgn.c:2743, on
gcc.c-torture/compile/20010102-1.c.  I haven't tried to sort that one
out yet.


Anyhow, should I try to fix these various checks, or should we follow
the safer approach here?

Ian