Re: Patch: Fix bug in combine

[Adrian Straetling <straetling at de dot ibm dot com>]

Ian Lance Taylor wrote:
> 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);
This patch causes a bootstrap failure on s390x
(HEAD + additional patch that adds a zero_extract pattern).

Consider the following situation: pos==32 && offset==0 && bfmode=HImode
after the lines above the values are: pos==0, offset==2.
But the offset should be 4 (bytes) as before the patch.

> > > 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.

There is a patch attached that solves the case for me.
Does it still solve your problem?

Ok for 4.1 and 4.0, once testing succeeded?

Bye,
Adrian


2005-10-11  Adrian Straetling <straetling@de.ibm.com>

* combine.c (make_extraction): Correct offset computation.


Index: gcc/combine.c
===================================================================
*** gcc/combine.c.orig	2005-10-09 13:46:52.000000000 +0200
--- gcc/combine.c	2005-10-11 23:12:33.764003958 +0200
*************** make_extraction (enum machine_mode mode,
*** 6485,6495 ****
  	offset -= GET_MODE_SIZE (is_mode) - GET_MODE_SIZE (inner_mode);
  
        /* 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);
  	}
  
--- 6485,6496 ----
  	offset -= GET_MODE_SIZE (is_mode) - GET_MODE_SIZE (inner_mode);
  
        /* If this is a constant position, we can move to the desired byte.
! 	 Be careful not to go beyond the original object and maintain the
! 	 natural alignment of the memory.  */ 
        if (pos_rtx == 0)
  	{
  	  enum machine_mode bfmode = smallest_mode_for_size (len, MODE_INT);
! 	  offset += (pos / GET_MODE_BITSIZE (bfmode)) * GET_MODE_SIZE (bfmode);
  	  pos %= GET_MODE_BITSIZE (bfmode);
  	}