| Message ID | 87y573kxse.fsf@talisman.default |
|---|---|
| State | New |
| Headers | show |
Hi Richard, Thanks I'll give a go tomorrow. Not sure why it has only been seen/reported for MIPS so far I can't see why it can't happen in other backends. Cheers Graham
On Wed, Sep 11, 2013 at 8:39 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Graham Stott <graham.stott@btinternet.com> writes: >> Hi Richard, >> >> There is some minor testsuite fallout with these patches on MIPS a >> couple of tests (see below)ICE ingen_int_mode () in both these ICE the >> mode is CCmode. > > Hmm, interesting. I suppose gen_int_mode should handle CC modes, > since there's no other constant rtx that can be used instead. OTOH, > like you say, it doesn't really make sense to apply try_const_anchor > to CCmode. Can we statically identify the places that build CCmode integers? In that case a gen_cc_const () function would be more appropriate ... Do we know that CCmode fits into a HWI btw? (what about partial integer modes which have weird precision (none)?) Thanks, Richard. > How does the following patch look? > > Thanks, > Richard > > > gcc/ > * emit-rtl.c (gen_int_mode): Handle CC modes. > * cse.c (try_const_anchors): ...but punt on them here. > > Index: gcc/emit-rtl.c > =================================================================== > --- gcc/emit-rtl.c 2013-09-08 11:52:15.000000000 +0100 > +++ gcc/emit-rtl.c 2013-09-11 19:32:35.702377902 +0100 > @@ -417,6 +417,11 @@ gen_rtx_CONST_INT (enum machine_mode mod > rtx > gen_int_mode (HOST_WIDE_INT c, enum machine_mode mode) > { > + /* CONST_INT is used for CC modes too. We can't make any assumptions > + about the precision or bitsize in that case, so just pass the value > + through unchanged. */ > + if (GET_MODE_CLASS (mode) == MODE_CC) > + return GEN_INT (c); > return GEN_INT (trunc_int_for_mode (c, mode)); > } > > Index: gcc/cse.c > =================================================================== > --- gcc/cse.c 2013-09-08 11:52:15.000000000 +0100 > +++ gcc/cse.c 2013-09-11 19:38:17.664399826 +0100 > @@ -1354,6 +1354,11 @@ try_const_anchors (rtx src_const, enum m > rtx lower_exp = NULL_RTX, upper_exp = NULL_RTX; > unsigned lower_old, upper_old; > > + /* CONST_INT is used for CC modes, but we should leave those alone. */ > + if (GET_MODE_CLASS (mode) == MODE_CC) > + return NULL_RTX; > + > + gcc_assert (SCALAR_INT_MODE_P (mode)); > if (!compute_const_anchors (src_const, &lower_base, &lower_offs, > &upper_base, &upper_offs)) > return NULL_RTX;
Richard Biener <richard.guenther@gmail.com> writes: > On Wed, Sep 11, 2013 at 8:39 PM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Graham Stott <graham.stott@btinternet.com> writes: >>> Hi Richard, >>> >>> There is some minor testsuite fallout with these patches on MIPS a >>> couple of tests (see below)ICE ingen_int_mode () in both these ICE the >>> mode is CCmode. >> >> Hmm, interesting. I suppose gen_int_mode should handle CC modes, >> since there's no other constant rtx that can be used instead. OTOH, >> like you say, it doesn't really make sense to apply try_const_anchor >> to CCmode. > > Can we statically identify the places that build CCmode integers? In that > case a gen_cc_const () function would be more appropriate ... I think it'd be hard to do statically. E.g. it wasn't really obvious that this particular cse code might have to handle CCmode. It might be better to have a different rtx for MODE_CC constants, hopefully then just using gen_rtx_CONST_CC (...) directly. It'd be a lot of work though. > Do we know that CCmode fits into a HWI btw? The precision of CCmode is effectively left undefined, so it's hard to say. But it always seems to have worked in practice. I suppose the most likely values are 0, 1 and -1, although MIPS did for a while have 1.0f as the CC value for "true". > (what about partial integer modes which have weird precision (none)?) We don't really model that properly yet. Partial modes are just defined using something like: PARTIAL_INT_MODE (SI); i.e. without the partial precision. But trunc_int_for_mode is based on GET_MODE_PRECISION, so should just work if a proper precision is added. (There are other places that wouldn't just work, but that's something that the wide-int branch will help with. It almost feels like you were setting me up for that answer. :-)) Thanks, Richard
On Thu, Sep 12, 2013 at 6:19 PM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >> On Wed, Sep 11, 2013 at 8:39 PM, Richard Sandiford >> <rdsandiford@googlemail.com> wrote: >>> Graham Stott <graham.stott@btinternet.com> writes: >>>> Hi Richard, >>>> >>>> There is some minor testsuite fallout with these patches on MIPS a >>>> couple of tests (see below)ICE ingen_int_mode () in both these ICE the >>>> mode is CCmode. >>> >>> Hmm, interesting. I suppose gen_int_mode should handle CC modes, >>> since there's no other constant rtx that can be used instead. OTOH, >>> like you say, it doesn't really make sense to apply try_const_anchor >>> to CCmode. >> >> Can we statically identify the places that build CCmode integers? In that >> case a gen_cc_const () function would be more appropriate ... > > I think it'd be hard to do statically. E.g. it wasn't really obvious > that this particular cse code might have to handle CCmode. > > It might be better to have a different rtx for MODE_CC constants, > hopefully then just using gen_rtx_CONST_CC (...) directly. > It'd be a lot of work though. > >> Do we know that CCmode fits into a HWI btw? > > The precision of CCmode is effectively left undefined, so it's hard to say. > But it always seems to have worked in practice. > > I suppose the most likely values are 0, 1 and -1, although MIPS did for > a while have 1.0f as the CC value for "true". > >> (what about partial integer modes which have weird precision (none)?) > > We don't really model that properly yet. Partial modes are just defined > using something like: > > PARTIAL_INT_MODE (SI); > > i.e. without the partial precision. But trunc_int_for_mode is based on > GET_MODE_PRECISION, so should just work if a proper precision is added. > (There are other places that wouldn't just work, but that's something > that the wide-int branch will help with. It almost feels like you were > setting me up for that answer. :-)) Well I was asking because if you change a GEN_INT (x) to gen_int_mode (PSImode, x) then you'll end up in trunc_int_for_mode with PSImode which looks at GET_MODE_PRECISION (PSImode is still a SCALAR_INT_MODE_P ...). We set precision of PSImode to -1U it seems, but that still means that the constant is not even canonicalized to the underlying mode. (so, should we set partial integer modes precision to the precision of the base mode? Well, of course we should simply set an appropriate precision at mode definition time) Richard. > Thanks, > Richard
Richard Biener <richard.guenther@gmail.com> writes: >>> (what about partial integer modes which have weird precision (none)?) >> >> We don't really model that properly yet. Partial modes are just defined >> using something like: >> >> PARTIAL_INT_MODE (SI); >> >> i.e. without the partial precision. But trunc_int_for_mode is based on >> GET_MODE_PRECISION, so should just work if a proper precision is added. >> (There are other places that wouldn't just work, but that's something >> that the wide-int branch will help with. It almost feels like you were >> setting me up for that answer. :-)) > > Well I was asking because if you change a GEN_INT (x) to > gen_int_mode (PSImode, x) then you'll end up in trunc_int_for_mode with > PSImode which looks at GET_MODE_PRECISION (PSImode is still > a SCALAR_INT_MODE_P ...). We set precision of PSImode to -1U > it seems, but that still means that the constant is not even canonicalized > to the underlying mode. (so, should we set partial integer modes precision > to the precision of the base mode? Well, of course we should simply set > an appropriate precision at mode definition time) Yeah. I don't think it makes sense to canonise PSI to 32 bits when we know it has fewer than 32 bits. It's still going to be wrong, and will still defeat one of the main purposes of canonical constants, which is to make equality obvious. E.g. we'd still treat PSI constants (const_int 0x...ff80000000) and (const_int 0x...ffc0000000) as different, even though they're both zero for a 24-bit PSI. This partial integer mode stuff is just a big hack. Like you say, we can only really do something sensible if we know what the real precision is. So I'd rather leave things as they are for this series. The precision effectively becomes 65535 thanks to the underlying unsigned short array, so like you say, trunc_int_for_mode is a no-op for partial modes. Changing GEN_INT to gen_int_mode shouldn't make any difference. What do you think about the patch for CC modes? Thanks, Richard
On Fri, Sep 13, 2013 at 10:08 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >>>> (what about partial integer modes which have weird precision (none)?) >>> >>> We don't really model that properly yet. Partial modes are just defined >>> using something like: >>> >>> PARTIAL_INT_MODE (SI); >>> >>> i.e. without the partial precision. But trunc_int_for_mode is based on >>> GET_MODE_PRECISION, so should just work if a proper precision is added. >>> (There are other places that wouldn't just work, but that's something >>> that the wide-int branch will help with. It almost feels like you were >>> setting me up for that answer. :-)) >> >> Well I was asking because if you change a GEN_INT (x) to >> gen_int_mode (PSImode, x) then you'll end up in trunc_int_for_mode with >> PSImode which looks at GET_MODE_PRECISION (PSImode is still >> a SCALAR_INT_MODE_P ...). We set precision of PSImode to -1U >> it seems, but that still means that the constant is not even canonicalized >> to the underlying mode. (so, should we set partial integer modes precision >> to the precision of the base mode? Well, of course we should simply set >> an appropriate precision at mode definition time) > > Yeah. I don't think it makes sense to canonise PSI to 32 bits when we > know it has fewer than 32 bits. It's still going to be wrong, and will > still defeat one of the main purposes of canonical constants, which is > to make equality obvious. E.g. we'd still treat PSI constants > > (const_int 0x...ff80000000) > > and > > (const_int 0x...ffc0000000) > > as different, even though they're both zero for a 24-bit PSI. > > This partial integer mode stuff is just a big hack. Like you say, > we can only really do something sensible if we know what the real > precision is. > > So I'd rather leave things as they are for this series. The precision > effectively becomes 65535 thanks to the underlying unsigned short array, > so like you say, trunc_int_for_mode is a no-op for partial modes. > Changing GEN_INT to gen_int_mode shouldn't make any difference. > > What do you think about the patch for CC modes? I wonder if we can do without changing gen_int_mode by fixing the callers like you did for cse.c (that patch is ok). Richard. > Thanks, > Richard
Richard Biener <richard.guenther@gmail.com> writes: >> Yeah. I don't think it makes sense to canonise PSI to 32 bits when we >> know it has fewer than 32 bits. It's still going to be wrong, and will >> still defeat one of the main purposes of canonical constants, which is >> to make equality obvious. E.g. we'd still treat PSI constants >> >> (const_int 0x...ff80000000) >> >> and >> >> (const_int 0x...ffc0000000) >> >> as different, even though they're both zero for a 24-bit PSI. >> >> This partial integer mode stuff is just a big hack. Like you say, >> we can only really do something sensible if we know what the real >> precision is. >> >> So I'd rather leave things as they are for this series. The precision >> effectively becomes 65535 thanks to the underlying unsigned short array, >> so like you say, trunc_int_for_mode is a no-op for partial modes. >> Changing GEN_INT to gen_int_mode shouldn't make any difference. >> >> What do you think about the patch for CC modes? > > I wonder if we can do without changing gen_int_mode by fixing the callers > like you did for cse.c (that patch is ok). But the point of the gen_int_mode patch is that (for better or worse) CONST_INT is the right choice of rtx for constant CC values. I'd like eventually to get rid of all GEN_INT callers, and some of those callers will sometimes or always be handling CC modes. This would then crop up again in a legitimate context. IMO, cse.c was buggy not because it was trying to generate a CCmode CONST_INT, but because anchor optimisations involve addition, which isn't defined for CC modes. I think gen_int_mode should do the right thing for all CONST_INT modes. gen_int_mode (X, MODE) should really be the moral equivalent of gen_rtx_CONST_INT (MODE, X), despite the confusingly-swapped parameters. Thanks, Richard
On Fri, Sep 13, 2013 at 10:33 AM, Richard Sandiford <rdsandiford@googlemail.com> wrote: > Richard Biener <richard.guenther@gmail.com> writes: >>> Yeah. I don't think it makes sense to canonise PSI to 32 bits when we >>> know it has fewer than 32 bits. It's still going to be wrong, and will >>> still defeat one of the main purposes of canonical constants, which is >>> to make equality obvious. E.g. we'd still treat PSI constants >>> >>> (const_int 0x...ff80000000) >>> >>> and >>> >>> (const_int 0x...ffc0000000) >>> >>> as different, even though they're both zero for a 24-bit PSI. >>> >>> This partial integer mode stuff is just a big hack. Like you say, >>> we can only really do something sensible if we know what the real >>> precision is. >>> >>> So I'd rather leave things as they are for this series. The precision >>> effectively becomes 65535 thanks to the underlying unsigned short array, >>> so like you say, trunc_int_for_mode is a no-op for partial modes. >>> Changing GEN_INT to gen_int_mode shouldn't make any difference. >>> >>> What do you think about the patch for CC modes? >> >> I wonder if we can do without changing gen_int_mode by fixing the callers >> like you did for cse.c (that patch is ok). > > But the point of the gen_int_mode patch is that (for better or worse) > CONST_INT is the right choice of rtx for constant CC values. I'd like > eventually to get rid of all GEN_INT callers, and some of those callers > will sometimes or always be handling CC modes. This would then crop up > again in a legitimate context. > > IMO, cse.c was buggy not because it was trying to generate a CCmode > CONST_INT, but because anchor optimisations involve addition, which > isn't defined for CC modes. > > I think gen_int_mode should do the right thing for all CONST_INT modes. > gen_int_mode (X, MODE) should really be the moral equivalent of > gen_rtx_CONST_INT (MODE, X), despite the confusingly-swapped parameters. Yeah, ok. But the GEN_INT changes have already uncovered quite some bugs so maybe let us discover a few more CCmode bugs first? ... Anyway, as you choose - the patch is ok in the end. Thanks, Richard. > Thanks, > Richard
Richard Biener <richard.guenther@gmail.com> writes: > On Fri, Sep 13, 2013 at 10:33 AM, Richard Sandiford > <rdsandiford@googlemail.com> wrote: >> Richard Biener <richard.guenther@gmail.com> writes: >>>> Yeah. I don't think it makes sense to canonise PSI to 32 bits when we >>>> know it has fewer than 32 bits. It's still going to be wrong, and will >>>> still defeat one of the main purposes of canonical constants, which is >>>> to make equality obvious. E.g. we'd still treat PSI constants >>>> >>>> (const_int 0x...ff80000000) >>>> >>>> and >>>> >>>> (const_int 0x...ffc0000000) >>>> >>>> as different, even though they're both zero for a 24-bit PSI. >>>> >>>> This partial integer mode stuff is just a big hack. Like you say, >>>> we can only really do something sensible if we know what the real >>>> precision is. >>>> >>>> So I'd rather leave things as they are for this series. The precision >>>> effectively becomes 65535 thanks to the underlying unsigned short array, >>>> so like you say, trunc_int_for_mode is a no-op for partial modes. >>>> Changing GEN_INT to gen_int_mode shouldn't make any difference. >>>> >>>> What do you think about the patch for CC modes? >>> >>> I wonder if we can do without changing gen_int_mode by fixing the callers >>> like you did for cse.c (that patch is ok). >> >> But the point of the gen_int_mode patch is that (for better or worse) >> CONST_INT is the right choice of rtx for constant CC values. I'd like >> eventually to get rid of all GEN_INT callers, and some of those callers >> will sometimes or always be handling CC modes. This would then crop up >> again in a legitimate context. >> >> IMO, cse.c was buggy not because it was trying to generate a CCmode >> CONST_INT, but because anchor optimisations involve addition, which >> isn't defined for CC modes. >> >> I think gen_int_mode should do the right thing for all CONST_INT modes. >> gen_int_mode (X, MODE) should really be the moral equivalent of >> gen_rtx_CONST_INT (MODE, X), despite the confusingly-swapped parameters. > > Yeah, ok. But the GEN_INT changes have already uncovered quite some bugs > so maybe let us discover a few more CCmode bugs first? ... OK, sounds fair. I'll hold off the gen_int_mode change until I have legitimate use. I went ahead and applied the cse.c change. Thanks, Richard
Index: gcc/emit-rtl.c =================================================================== --- gcc/emit-rtl.c 2013-09-08 11:52:15.000000000 +0100 +++ gcc/emit-rtl.c 2013-09-11 19:32:35.702377902 +0100 @@ -417,6 +417,11 @@ gen_rtx_CONST_INT (enum machine_mode mod rtx gen_int_mode (HOST_WIDE_INT c, enum machine_mode mode) { + /* CONST_INT is used for CC modes too. We can't make any assumptions + about the precision or bitsize in that case, so just pass the value + through unchanged. */ + if (GET_MODE_CLASS (mode) == MODE_CC) + return GEN_INT (c); return GEN_INT (trunc_int_for_mode (c, mode)); } Index: gcc/cse.c =================================================================== --- gcc/cse.c 2013-09-08 11:52:15.000000000 +0100 +++ gcc/cse.c 2013-09-11 19:38:17.664399826 +0100 @@ -1354,6 +1354,11 @@ try_const_anchors (rtx src_const, enum m rtx lower_exp = NULL_RTX, upper_exp = NULL_RTX; unsigned lower_old, upper_old; + /* CONST_INT is used for CC modes, but we should leave those alone. */ + if (GET_MODE_CLASS (mode) == MODE_CC) + return NULL_RTX; + + gcc_assert (SCALAR_INT_MODE_P (mode)); if (!compute_const_anchors (src_const, &lower_base, &lower_offs, &upper_base, &upper_offs)) return NULL_RTX;