[wide-int] Fix mode choice in convert_modes

Submitted by Richard Sandiford on Sept. 9, 2013, 1:11 p.m.

Details

Message ID 871u4ygmwq.fsf@talisman.default
State New
Headers show

Commit Message

Richard Sandiford Sept. 9, 2013, 1:11 p.m.
Enabling the CONST_INT assert showed that this branch-local code was
passing the wrong mode to std::make_pair.  The mode of X is OLDMODE,
and we're supposed to be converting it to MODE.

In the case where OLDMODE is VOIDmode, I think we have to assume
that every bit of the input is significant.

Tested on x86_64-linux-gnu.  OK to install?

This and the other patches that I just committed to trunk are enough
for a bootstrap and regression test to pass on x86_64-linux-gnu with
the assertion for a canonical CONST_INT enabled.  I think it would be
better to leave the assertion out until after the merge though,
so we can deal with any fallout separately.

Thanks,
Richard

Comments

Richard Guenther Sept. 9, 2013, 1:23 p.m.
On Mon, Sep 9, 2013 at 3:11 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Enabling the CONST_INT assert showed that this branch-local code was
> passing the wrong mode to std::make_pair.  The mode of X is OLDMODE,
> and we're supposed to be converting it to MODE.
>
> In the case where OLDMODE is VOIDmode, I think we have to assume
> that every bit of the input is significant.
>
> Tested on x86_64-linux-gnu.  OK to install?
>
> This and the other patches that I just committed to trunk are enough
> for a bootstrap and regression test to pass on x86_64-linux-gnu with
> the assertion for a canonical CONST_INT enabled.  I think it would be
> better to leave the assertion out until after the merge though,
> so we can deal with any fallout separately.

If consuming code assumes CONST_INTs are canonical we better leave it
in though.

Richard.

> Thanks,
> Richard
>
>
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c  2013-09-09 11:32:33.734617409 +0100
> +++ gcc/expr.c  2013-09-09 11:45:27.381001353 +0100
> @@ -714,13 +714,14 @@ convert_modes (enum machine_mode mode, e
>        && GET_MODE_CLASS (mode) == MODE_INT
>        && (oldmode == VOIDmode || GET_MODE_CLASS (oldmode) == MODE_INT))
>      {
> -      wide_int w = std::make_pair (x, mode);
>        /* If the caller did not tell us the old mode, then there is
> -        not much to do with respect to canonization.  */
> -      if (oldmode != VOIDmode
> -         && GET_MODE_PRECISION (mode) > GET_MODE_PRECISION (oldmode))
> -       w = wi::ext (w, GET_MODE_PRECISION (oldmode),
> -                    unsignedp ? UNSIGNED : SIGNED);
> +        not much to do with respect to canonization.  We have to assume
> +        that all the bits are significant.  */
> +      if (oldmode == VOIDmode)
> +       oldmode = MAX_MODE_INT;
> +      wide_int w = wide_int::from (std::make_pair (x, oldmode),
> +                                  GET_MODE_PRECISION (mode),
> +                                  unsignedp ? UNSIGNED : SIGNED);
>        return immed_wide_int_const (w, mode);
>      }
>
Richard Sandiford Sept. 9, 2013, 1:56 p.m.
Richard Biener <richard.guenther@gmail.com> writes:
> On Mon, Sep 9, 2013 at 3:11 PM, Richard Sandiford
> <rdsandiford@googlemail.com> wrote:
>> Enabling the CONST_INT assert showed that this branch-local code was
>> passing the wrong mode to std::make_pair.  The mode of X is OLDMODE,
>> and we're supposed to be converting it to MODE.
>>
>> In the case where OLDMODE is VOIDmode, I think we have to assume
>> that every bit of the input is significant.
>>
>> Tested on x86_64-linux-gnu.  OK to install?
>>
>> This and the other patches that I just committed to trunk are enough
>> for a bootstrap and regression test to pass on x86_64-linux-gnu with
>> the assertion for a canonical CONST_INT enabled.  I think it would be
>> better to leave the assertion out until after the merge though,
>> so we can deal with any fallout separately.
>
> If consuming code assumes CONST_INTs are canonical we better leave it
> in though.

Yeah, I think the patch Kenny's working on is going to canonise CONST_INTs
via the scratch array, to avoid making any assumptions.  I think we should
keep that for the merge and only swap it for an assert later.

Thanks,
Richard
Kenneth Zadeck Sept. 9, 2013, 2:05 p.m.
On 09/09/2013 09:56 AM, Richard Sandiford wrote:
> Richard Biener <richard.guenther@gmail.com> writes:
>> On Mon, Sep 9, 2013 at 3:11 PM, Richard Sandiford
>> <rdsandiford@googlemail.com> wrote:
>>> Enabling the CONST_INT assert showed that this branch-local code was
>>> passing the wrong mode to std::make_pair.  The mode of X is OLDMODE,
>>> and we're supposed to be converting it to MODE.
>>>
>>> In the case where OLDMODE is VOIDmode, I think we have to assume
>>> that every bit of the input is significant.
>>>
>>> Tested on x86_64-linux-gnu.  OK to install?
>>>
>>> This and the other patches that I just committed to trunk are enough
>>> for a bootstrap and regression test to pass on x86_64-linux-gnu with
>>> the assertion for a canonical CONST_INT enabled.  I think it would be
>>> better to leave the assertion out until after the merge though,
>>> so we can deal with any fallout separately.
>> If consuming code assumes CONST_INTs are canonical we better leave it
>> in though.
> Yeah, I think the patch Kenny's working on is going to canonise CONST_INTs
> via the scratch array, to avoid making any assumptions.  I think we should
> keep that for the merge and only swap it for an assert later.
>
> Thanks,
> Richard
yes, this is what my patch does now.
Richard Sandiford Sept. 18, 2013, 7:27 a.m.
Ping

Richard Sandiford <rdsandiford@googlemail.com> writes:
> Enabling the CONST_INT assert showed that this branch-local code was
> passing the wrong mode to std::make_pair.  The mode of X is OLDMODE,
> and we're supposed to be converting it to MODE.
>
> In the case where OLDMODE is VOIDmode, I think we have to assume
> that every bit of the input is significant.
>
> Tested on x86_64-linux-gnu.  OK to install?
>
> This and the other patches that I just committed to trunk are enough
> for a bootstrap and regression test to pass on x86_64-linux-gnu with
> the assertion for a canonical CONST_INT enabled.  I think it would be
> better to leave the assertion out until after the merge though,
> so we can deal with any fallout separately.
>
> Thanks,
> Richard
>
>
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c	2013-09-09 11:32:33.734617409 +0100
> +++ gcc/expr.c	2013-09-09 11:45:27.381001353 +0100
> @@ -714,13 +714,14 @@ convert_modes (enum machine_mode mode, e
>        && GET_MODE_CLASS (mode) == MODE_INT
>        && (oldmode == VOIDmode || GET_MODE_CLASS (oldmode) == MODE_INT))
>      {
> -      wide_int w = std::make_pair (x, mode);
>        /* If the caller did not tell us the old mode, then there is
> -	 not much to do with respect to canonization.  */
> -      if (oldmode != VOIDmode
> -	  && GET_MODE_PRECISION (mode) > GET_MODE_PRECISION (oldmode))
> -	w = wi::ext (w, GET_MODE_PRECISION (oldmode),
> -		     unsignedp ? UNSIGNED : SIGNED);
> +	 not much to do with respect to canonization.  We have to assume
> +	 that all the bits are significant.  */
> +      if (oldmode == VOIDmode)
> +	oldmode = MAX_MODE_INT;
> +      wide_int w = wide_int::from (std::make_pair (x, oldmode),
> +				   GET_MODE_PRECISION (mode),
> +				   unsignedp ? UNSIGNED : SIGNED);
>        return immed_wide_int_const (w, mode);
>      }
>
Kenneth Zadeck Sept. 18, 2013, 11:12 a.m.
this is fine with me.

kenny

On 09/18/2013 03:27 AM, Richard Sandiford wrote:
> Ping
>
> Richard Sandiford <rdsandiford@googlemail.com> writes:
>> Enabling the CONST_INT assert showed that this branch-local code was
>> passing the wrong mode to std::make_pair.  The mode of X is OLDMODE,
>> and we're supposed to be converting it to MODE.
>>
>> In the case where OLDMODE is VOIDmode, I think we have to assume
>> that every bit of the input is significant.
>>
>> Tested on x86_64-linux-gnu.  OK to install?
>>
>> This and the other patches that I just committed to trunk are enough
>> for a bootstrap and regression test to pass on x86_64-linux-gnu with
>> the assertion for a canonical CONST_INT enabled.  I think it would be
>> better to leave the assertion out until after the merge though,
>> so we can deal with any fallout separately.
>>
>> Thanks,
>> Richard
>>
>>
>> Index: gcc/expr.c
>> ===================================================================
>> --- gcc/expr.c	2013-09-09 11:32:33.734617409 +0100
>> +++ gcc/expr.c	2013-09-09 11:45:27.381001353 +0100
>> @@ -714,13 +714,14 @@ convert_modes (enum machine_mode mode, e
>>         && GET_MODE_CLASS (mode) == MODE_INT
>>         && (oldmode == VOIDmode || GET_MODE_CLASS (oldmode) == MODE_INT))
>>       {
>> -      wide_int w = std::make_pair (x, mode);
>>         /* If the caller did not tell us the old mode, then there is
>> -	 not much to do with respect to canonization.  */
>> -      if (oldmode != VOIDmode
>> -	  && GET_MODE_PRECISION (mode) > GET_MODE_PRECISION (oldmode))
>> -	w = wi::ext (w, GET_MODE_PRECISION (oldmode),
>> -		     unsignedp ? UNSIGNED : SIGNED);
>> +	 not much to do with respect to canonization.  We have to assume
>> +	 that all the bits are significant.  */
>> +      if (oldmode == VOIDmode)
>> +	oldmode = MAX_MODE_INT;
>> +      wide_int w = wide_int::from (std::make_pair (x, oldmode),
>> +				   GET_MODE_PRECISION (mode),
>> +				   unsignedp ? UNSIGNED : SIGNED);
>>         return immed_wide_int_const (w, mode);
>>       }
>>

Patch hide | download patch | download mbox

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	2013-09-09 11:32:33.734617409 +0100
+++ gcc/expr.c	2013-09-09 11:45:27.381001353 +0100
@@ -714,13 +714,14 @@  convert_modes (enum machine_mode mode, e
       && GET_MODE_CLASS (mode) == MODE_INT
       && (oldmode == VOIDmode || GET_MODE_CLASS (oldmode) == MODE_INT))
     {
-      wide_int w = std::make_pair (x, mode);
       /* If the caller did not tell us the old mode, then there is
-	 not much to do with respect to canonization.  */
-      if (oldmode != VOIDmode
-	  && GET_MODE_PRECISION (mode) > GET_MODE_PRECISION (oldmode))
-	w = wi::ext (w, GET_MODE_PRECISION (oldmode),
-		     unsignedp ? UNSIGNED : SIGNED);
+	 not much to do with respect to canonization.  We have to assume
+	 that all the bits are significant.  */
+      if (oldmode == VOIDmode)
+	oldmode = MAX_MODE_INT;
+      wide_int w = wide_int::from (std::make_pair (x, oldmode),
+				   GET_MODE_PRECISION (mode),
+				   unsignedp ? UNSIGNED : SIGNED);
       return immed_wide_int_const (w, mode);
     }