Patch: Fix bug in combine

[Ian Lance Taylor]

Dale Johannesen <dalej@apple.com> writes:

> On Aug 26, 2005, at 12:15 PM, Ian Lance Taylor wrote:
> >> !       /* If this is a constant position, we can move to the
> >> desired byte.
> >> ! 	 Be careful not to go beyond the original object. */
> >>         if (pos_rtx == 0)
> >>   	{
> >> ! 	  enum machine_mode bfmode = smallest_mode_for_size (len,
> >> MODE_INT);
> >> ! 	  offset += pos / GET_MODE_BITSIZE (bfmode);
> >> ! 	  pos %= GET_MODE_BITSIZE (bfmode);
> >>   	}
> >
> > The comment and the ChangeLog entry aren't right.  What my patch
> > really does is maintain the natural alignment of the memory holding
> > the bitfield, if it was present to begin with.  That might well be a
> > good idea, but I can't convince myself that it will reliably fix the
> > bug in all cases.
> 
> I think it will, at least if all the sizes involved are powers of 2,
> and surely
> we can assume that?  For example, if bfmode is HImode, offset won't be
> advanced by more than 2 bytes and the new HImode reference will
> lie entirely within the original object.  What do you think wouldn't
> work?

The problem I have with that argument is that the final mode reference
is SImode, not HImode.

After looking into it further, the mode that matters here on i386 is
really the mode for pos + len, using the final value for pos.  That is
what is used by the define_split around line 7920 of i386.md.  In this
case len is 14 and pos starts at 14.  The bad case before the patch is
when pos > 8 (causing an increment) and (pos % 8) + len > 16 (causing
the access to use SImode) and pos + len < 32 (or we would wrap into
the next word anyhow).  Solving, the bad case is 24 < pos + len < 32,
which is indeed true in your test case.

To put it another way, we are in trouble if pos / N > 0 && pos % N +
len gives us the same mode as pos + len.  When that happens, we are
walking off the end of the field.  So we have to choose N such that
pos - N + len does not give us the same mode as pos + len.  That
should always be true if N >= MODE_SIZE (len).

So, OK, the patch looks good.  I won't approve it, though, since I
wrote it.

Thanks.  By the way, I don't think you included your new test case in
your patch submission, unless I missed it somewhere.

Ian