Re: [patch] More thorough checking in reg_fits_class_p

[Georg-Johann Lay <avr at gjlay dot de>]

Richard Earnshaw schrieb:
> On 30/04/12 15:07, Richard Sandiford wrote:
>
> > Richard Earnshaw  writes:
> >
> > > Jim MacArthur wrote:
> > >
> > > > The current code in reg_fits_class_p appears to be incorrect; since 
> > > > offset may be negative, it's necessary to check both ends of the range 
> > > > otherwise an array overrun or underrun may occur when calling 
> > > > in_hard_reg_set_p. in_hard_reg_set_p should also be checked for each 
> > > > register in the range of regno .. regno+offset.
> > > >
> > > > A negative offset can occur on a big-endian target. For example, on 
> > > > AArch64, subreg_regno_offset (0, DImode, 0, TImode) returns -1.
> > > >
> > > > We discovered this problem while developing unrelated code for 
> > > > big-endian support in the AArch64 back end.
> > > >
> > > > I've tested this with an x86 bootstrap which shows no errors, and with 
> > > > our own AArch64 back end.
> > > >
> > > > Ok for trunk?
> > > >
> > > > gcc/Changelog entry:
> > > >
> > > > 2012-04-26 Jim MacArthur<jim.macarthur@arm.com>
> > > >      * recog.c (reg_fits_class_p): Check every register between regno and
> > > >        regno+offset is in the hard register set.
> > >
> > > OK.
> > >
> > > R.
> > >
> > > > reg-fits-class-9
> > > >
> > > > diff --git a/gcc/recog.c b/gcc/recog.c
> > > > index 8fb96a0..825bfb1 100644
> > > > --- a/gcc/recog.c
> > > > +++ b/gcc/recog.c
> > > > @@ -2759,14 +2759,28 @@ bool
> > > > reg_fits_class_p (const_rtx operand, reg_class_t cl, int offset,
> > > > 		  enum machine_mode mode)
> > > > {
> > > > -  int regno = REGNO (operand);
> > > > +  unsigned int regno = REGNO (operand);
> > > >
> > > >   if (cl == NO_REGS)
> > > >     return false;
> > > >
> > > > -  return (HARD_REGISTER_NUM_P (regno)
> > > > -	  && in_hard_reg_set_p (reg_class_contents[(int) cl],
> > > > -				mode, regno + offset));
> > > > +  /* We should not assume all registers in the range regno to regno + offset are
> > > > +     valid just because each end of the range is.  */
> > > > +  if (HARD_REGISTER_NUM_P (regno) && HARD_REGISTER_NUM_P (regno + offset))
> > > > +    {
> > > > +      unsigned int i;
> > > > +
> > > > +      unsigned int start = MIN (regno, regno + offset);
> > > > +      unsigned int end = MAX (regno, regno + offset);
> > > > +      for (i = start; i <= end; i++)
> > > > +	{
> > > > +	  if (!in_hard_reg_set_p (reg_class_contents[(int) cl],
> > > > +				  mode, i))
> > > > +	    return false;
> > > > +	}
> >
> > This doesn't look right to me.  We should still only need to check
> > in_hard_reg_set_p for one register number.  I'd have expected
> > something like:
> >
> >  return (HARD_REGISTER_NUM_P (regno)
> > 	  && HARD_REGISTER_NUM_P (regno + offset)
> > 	  && in_hard_reg_set_p (reg_class_contents[(int) cl],
> > 				mode, regno + offset));
> >
> > instead.
> >
> > Richard
>
> Hmm, looking at the comment that precedes the function makes me think
> the actual implementation should be:
>
> {
> 	int regno = REGNO (operand) + offset;
> 	...
>
> 	return (HARD_REGISTER_NUM_P (regno)
> 		&& HARD_REGISTER_NUM_P (end_hard_regno (regno, mode))
> 		&& in_hard_reg_set_p (...., regno);
>
> }
>
> That is, the original regno isn't really interesting and what really
> counts is the range regno + offset ... regno + offset +
> NUM_HARD_REGS(mode) - 1

Shouldn't this be HARD_REGNO_NREGS?

Johann