Patchwork [wide-int] Fix mode choice in convert_modes

login
register
mail settings
Submitter Richard Sandiford
Date Sept. 9, 2013, 1:11 p.m.
Message ID <871u4ygmwq.fsf@talisman.default>
Download mbox | patch
Permalink /patch/273564/
State New
Headers show

Comments

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

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);
     }