Re: Potential bug with wide_int_storage::set_len

[Eric Botcazou]

> If set_len really does 'ignore' signedness, then we are not treating X
> as having signedness SGN.

It does not, my interpretation is that it sign-extends when the value is not 
already sign-extended, probably for the reason exposed by Richard.  But that's 
admittedly quite confusing so a comment by the author would be in order.

-- 
Eric Botcazou

Re: Potential bug with wide_int_storage::set_len

[Richard Biener]

On Thu, Oct 13, 2016 at 10:34 AM, Andre Vieira (lists)
 wrote:
>
>> That is correct.  In RTL constants are always sign-extended from their
>> precision to HOST_BITS_PER_WIDE_INT, regardless if it is "signed" or
>> "unsigned" constant.  Whether you treat the low precision bits of the
>> constant as signed or unsigned is something encoded in the operation on it.
>>
>>   Jakub
>>
> Euhm, but then surely we must get rid of the is_sign_extended parameter
> altogether?
>
> Right now if you call that function for the same example, but with
> is_sign_extended set to true, the value will __NOT__ be sign extended.

I believe that is an optimization to cater for the difference between
the internal
storage used for INTEGER_CST trees and CONST_INT RTLs.  INTEGER_CSTs
representation is _not_ sign-extended (but it carries precision and
sign information).

Both CONST_INT and INTEGER_CST can be accessed as wide-ints without
copying the storage via some clever adaptor use but users need to care about
this subtle difference.

Richard.

Re: Potential bug with wide_int_storage::set_len

[Andre Vieira (lists)]

On 12/10/16 18:59, Eric Botcazou wrote:
>> During the development of a patch I encountered some strange behavior
>> and decided to investigate. The result of which is I think I found a bug
>> with 'wide_int_storage::set_len' in gcc/wide-int.h.
>>
>> The function reads:
>> inline void
>> wide_int_storage::set_len (unsigned int l, bool is_sign_extended)
>> {
>>   len = l;
>>   if (!is_sign_extended && len * HOST_BITS_PER_WIDE_INT > precision)
>> val[len - 1] = sext_hwi (val[len - 1],
>>  precision % HOST_BITS_PER_WIDE_INT);
>> }
> 
> The code certainly lacks a comment explaining the apparent discrepancy.

It could do some comments yes. Also, it might also be worth changing the
comments on 'wide_int_storage::from':

/* Treat X as having signedness SGN and convert it to a PRECISION-bit
   number.  */
inline wide_int
wide_int_storage::from (const wide_int_ref , unsigned int precision,
signop sgn)
{
  wide_int result = wide_int::create (precision);
  result.set_len (wi::force_to_size (result.write_val (), x.val, x.len,
 x.precision, precision, sgn));
  return result;
}

If set_len really does 'ignore' signedness, then we are not treating X
as having signedness SGN. Right?

Cheers,
Andre

Re: Potential bug with wide_int_storage::set_len

[Andre Vieira (lists)]

> That is correct.  In RTL constants are always sign-extended from their
> precision to HOST_BITS_PER_WIDE_INT, regardless if it is "signed" or
> "unsigned" constant.  Whether you treat the low precision bits of the
> constant as signed or unsigned is something encoded in the operation on it.
> 
>   Jakub
> 
Euhm, but then surely we must get rid of the is_sign_extended parameter
altogether?

Right now if you call that function for the same example, but with
is_sign_extended set to true, the value will __NOT__ be sign extended.

Re: Potential bug with wide_int_storage::set_len

[Eric Botcazou]

> During the development of a patch I encountered some strange behavior
> and decided to investigate. The result of which is I think I found a bug
> with 'wide_int_storage::set_len' in gcc/wide-int.h.
> 
> The function reads:
> inline void
> wide_int_storage::set_len (unsigned int l, bool is_sign_extended)
> {
>   len = l;
>   if (!is_sign_extended && len * HOST_BITS_PER_WIDE_INT > precision)
> val[len - 1] = sext_hwi (val[len - 1],
>  precision % HOST_BITS_PER_WIDE_INT);
> }

The code certainly lacks a comment explaining the apparent discrepancy.

> Due to this, 'expand_expr' will expand a constant tree with unsigned
> integer type and value MAX_UINT to a rtx node (const_int -1).

As Jakub explained, that is as expected, even if a little surprising.

-- 
Eric Botcazou

Re: Potential bug with wide_int_storage::set_len

[Jakub Jelinek]

On Wed, Oct 12, 2016 at 06:45:39PM +0200, Jakub Jelinek wrote:
> On Wed, Oct 12, 2016 at 05:02:46PM +0100, Andre Vieira (lists) wrote:
> > Say you are running it on a 64-bit host:
> > #define HOST_BITS_PER_WIDE_INT 64
> > 
> > and you call 'result.set_len (1, false);'
> > 
> > Then this will sign extend the first element of val, to
> > 0x, and I don't think this is what you want.
> > 
> > Due to this, 'expand_expr' will expand a constant tree with unsigned
> > integer type and value MAX_UINT to a rtx node (const_int -1).
> 
> That is correct.  In RTL constants are always sign-extended from their
> precision to HOST_BITS_PER_WIDE_INT, regardless if it is "signed" or
> "unsigned" constant.  Whether you treat the low precision bits of the
> constant as signed or unsigned is something encoded in the operation on it.

Here is what rtl.texi says on it:
@item (const_int @var{i})
This type of expression represents the integer value @var{i}.  @var{i}
is customarily accessed with the macro @code{INTVAL} as in
@code{INTVAL (@var{exp})}, which is equivalent to @code{XWINT (@var{exp}, 0)}.

Constants generated for modes with fewer bits than in
@code{HOST_WIDE_INT} must be sign extended to full width (e.g., with
@code{gen_int_mode}).  For constants for modes with more bits than in
@code{HOST_WIDE_INT} the implied high order bits of that constant are
copies of the top bit.  Note however that values are neither
inherently signed nor inherently unsigned; where necessary, signedness
is determined by the rtl operation instead.

Jakub

Re: Potential bug with wide_int_storage::set_len

[Jakub Jelinek]

On Wed, Oct 12, 2016 at 05:02:46PM +0100, Andre Vieira (lists) wrote:
> Say you are running it on a 64-bit host:
> #define HOST_BITS_PER_WIDE_INT 64
> 
> and you call 'result.set_len (1, false);'
> 
> Then this will sign extend the first element of val, to
> 0x, and I don't think this is what you want.
> 
> Due to this, 'expand_expr' will expand a constant tree with unsigned
> integer type and value MAX_UINT to a rtx node (const_int -1).

That is correct.  In RTL constants are always sign-extended from their
precision to HOST_BITS_PER_WIDE_INT, regardless if it is "signed" or
"unsigned" constant.  Whether you treat the low precision bits of the
constant as signed or unsigned is something encoded in the operation on it.

Jakub

Potential bug with wide_int_storage::set_len

[Andre Vieira (lists)]

Hello,

During the development of a patch I encountered some strange behavior
and decided to investigate. The result of which is I think I found a bug
with 'wide_int_storage::set_len' in gcc/wide-int.h.

The function reads:
inline void
wide_int_storage::set_len (unsigned int l, bool is_sign_extended)
{
  len = l;
  if (!is_sign_extended && len * HOST_BITS_PER_WIDE_INT > precision)
val[len - 1] = sext_hwi (val[len - 1],
 precision % HOST_BITS_PER_WIDE_INT);
}

Now assume you have a wide_int_storage, lets call it 'result' with the
following values:
val = [ 0x, ...];
len = 1;
precision = 32;

Say you are running it on a 64-bit host:
#define HOST_BITS_PER_WIDE_INT 64

and you call 'result.set_len (1, false);'

Then this will sign extend the first element of val, to
0x, and I don't think this is what you want.

Due to this, 'expand_expr' will expand a constant tree with unsigned
integer type and value MAX_UINT to a rtx node (const_int -1).

Am I missing something here?

Cheers,
Andre

PS: I will be running tests with a patch to remove the negation in front
of 'is_sign_extended' and post the patch in gcc-patches. If anyone
thinks this is wrong and wants to spare me the effort please reply!

Potential bug with wide_int_storage::set_len

[Andre Vieira (lists)]

Hello,

During the development of a patch I encountered some strange behavior
and decided to investigate. The result of which is I think I found a bug
with 'wide_int_storage::set_len' in gcc/wide-int.h.

The function reads:
inline void
wide_int_storage::set_len (unsigned int l, bool is_sign_extended)
{
  len = l;
  if (!is_sign_extended && len * HOST_BITS_PER_WIDE_INT > precision)
val[len - 1] = sext_hwi (val[len - 1],
 precision % HOST_BITS_PER_WIDE_INT);
}

Now assume you have a wide_int_storage, lets call it 'result' with the
following values:
val = [ 0x, ...];
len = 1;
precision = 32;

Say you are running it on a 64-bit host:
#define HOST_BITS_PER_WIDE_INT 64

and you call 'result.set_len (1, false);'

Then this will sign extend the first element of val, to
0x, and I don't think this is what you want.

Due to this, 'expand_expr' will expand a constant tree with unsigned
integer type and value MAX_UINT to a rtx node (const_int -1).

Am I missing something here?

Cheers,
Andre

PS: I will be running tests with a patch to remove the negation in front
of 'is_sign_extended' and post the patch in gcc-patches. If anyone
thinks this is wrong and wants to spare me the effort please reply!