| Message ID | 871u4ygmwq.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
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 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
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.
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); > } >
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); >> } >>
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); }