Patchwork Using gen_int_mode instead of GEN_INT minot testsuite fallout on MIPS

login
register
mail settings
Submitter Richard Sandiford
Date Sept. 11, 2013, 6:39 p.m.
Message ID <87y573kxse.fsf@talisman.default>
Download mbox | patch
Permalink /patch/274352/
State New
Headers show

Comments

Richard Sandiford - Sept. 11, 2013, 6:39 p.m.
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.

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.
Graham Stott - Sept. 11, 2013, 6:57 p.m.
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
Richard Guenther - Sept. 12, 2013, 8:07 a.m.
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 Sandiford - Sept. 12, 2013, 4:19 p.m.
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
Richard Guenther - Sept. 13, 2013, 7:42 a.m.
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 Sandiford - Sept. 13, 2013, 8:08 a.m.
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
Richard Guenther - Sept. 13, 2013, 8:12 a.m.
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 Sandiford - Sept. 13, 2013, 8:33 a.m.
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
Richard Guenther - Sept. 13, 2013, 8:43 a.m.
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 Sandiford - Sept. 16, 2013, 7:24 a.m.
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

Patch

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;