Re: PATCH: PR middle-end/35897: DSE doesn't support targets with wide ?registers

[Kenneth Zadeck <zadeck at naturalbridge dot com>]

H.J. Lu wrote:
> On Fri, Apr 11, 2008 at 10:14:42AM -0400, Kenneth Zadeck wrote:
>   
> > H.J. Lu wrote:
> >     
> > > On Fri, Apr 11, 2008 at 10:17:01AM +0200, Eric Botcazou wrote:
> > >   
> > >       
> > > > > 1. positions_needed is long. But mask, which is int, is used on it.
> > > > > long may not be the same as int.  We haven't seen problems on x86
> > > > > so far since SSE register is 16byte which is less than size of int
> > > > > * 8.  This is no longer true for AVX register, which is 32byte.
> > > > > 2. A 4-byte integer should be able handle 32byte register. The only
> > > > > problem is DSE uses (1L << width) - 1) to set bits 0 to width - 1
> > > > > in a bitmask to 1. 1L << 32 doesn't work with 4byte integer.
> > > > >
> > > > > This patch changes int to long for mask and introduces a new function,
> > > > > fill_bitmask, to return a bitmask with bits 0 to width - 1 set to 1.
> > > > > fill_bitmask works with width from 1 to 32. Does it make sense?
> > > > >       
> > > > >           
> > > > Yes, but use 'unsigned HOST_WIDE_INT' for positions_needed, mask and the 
> > > > return value of the new helper routine, HOST_BITS_PER_WIDE_INT instead of 
> > > > sizeof * CHAR_BIT and remove the FIXME.
> > > >
> > > > /* Return a bitmask with the first N low bits set.  */
> > > >  static unsigned HOST_WIDE_INT
> > > > lowpart_bitmask (int n)
> > > > {
> > > >   unsigned HOST_WIDE_INT mask = ~(unsigned HOST_WIDE_INT) 0;
> > > >   return mask >> (HOST_BITS_PER_WIDE_INT - n);
> > > > }
> > > >
> > > > OK with this change.
> > > >
> > > >     
> > > >         
> > > I have a concern to change bitmask to unsigned. There is
> > >
> > >                   if (store_info->rhs
> > >                       && (offset >= store_info->begin)
> > >                       && (offset + width <= store_info->end))
> > >                     {
> > >                       int mask = ((1L << width) - 1) << (offset - store_info->begin);
> > >                            if ((store_info->positions_needed & mask) == 
> > > mask                           && replace_read (store_info, i_ptr,         
> > >                                    read_info, insn_info, loc))
> > >                         return 0;
> > >                     }
> > >
> > > Kenny, you wrote dse.c.  Will changing it to unsigned make a
> > > difference between signe and unsigned?
> > >
> > > Thanks.
> > >
> > >
> > > H.J.
> > >   
> > >       
> > I should point out that it was bonzini who told me that this was ok. 
> > however your patch is close, but it needs a little more work.   doing the 
> > unsigned HOST_WIDEST_INT thing is fine, it gets you another bit and there 
> > are no places where the signness matters, but you need to track down more 
> > of the code.   in particular, the code that sets positions needed needs to 
> > be compatible with this wider type.
> >
> > 1) There are several local variables named "mask" that need to be the same 
> > unsigned HOST_WIDEST_INT
> >
> > 2) Also (and this is a place where a person who understands c a lot better 
> > than me needs to audit the code), there are lines like:
> >
> > s_info->positions_needed &= ~(1L << (i - s_info->begin));
> >
> > Does the "1L" need to a HOST_WIDEST_INT 1.   I do not know how to do this. 
> >     
>
> Hi Kenny,
>
> My patch is at
>
> http://gcc.gnu.org/ml/gcc-patches/2008-04/msg00937.html
>
> Does it look OK?
>
> Thanks.
>
>
> H.J.
>   
sorry, i did not see the entire patch.   it does look ok.