Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
On Tue, 18 Dec 2018 03:08:51 -0600
Segher Boessenkool wrote:
> Hi!
>
> On Fri, Dec 14, 2018 at 03:22:13PM +, Jozef Lawrynowicz wrote:
> > 2018-12-14 Jozef Lawrynowicz
> >
> > gcc/ChangeLog:
> > * combine.c (update_rsp_from_reg_equal): Only look for the nonzero bits
> > of src in nonzero_bits_mode if the mode of src is MODE_INT and
> > HWI_COMPUTABLE.
> > (reg_nonzero_bits_for_combine): Add clarification to comment.
>
> Is there some PR this fixes?
No not for this one, I just spotted the timeouts in the GCC testsuite.
> >
> > diff --git a/gcc/combine.c b/gcc/combine.c
> > index 7e61139..c93aaed 100644
> > --- a/gcc/combine.c
> > +++ b/gcc/combine.c
> > @@ -1698,9 +1698,13 @@ update_rsp_from_reg_equal (reg_stat_type *rsp,
> > rtx_insn *insn, const_rtx set,
> >/* Don't call nonzero_bits if it cannot change anything. */
> >if (rsp->nonzero_bits != HOST_WIDE_INT_M1U)
> > {
> > - bits = nonzero_bits (src, nonzero_bits_mode);
> > + machine_mode mode = GET_MODE (x);
> > + if (GET_MODE_CLASS (mode) == MODE_INT
> > + && HWI_COMPUTABLE_MODE_P (mode))
> > + mode = nonzero_bits_mode;
> > + bits = nonzero_bits (src, mode);
> >if (reg_equal && bits)
> > - bits &= nonzero_bits (reg_equal, nonzero_bits_mode);
> > + bits &= nonzero_bits (reg_equal, mode);
> >rsp->nonzero_bits |= bits;
> > }
> >
> > @@ -10224,6 +10228,7 @@ simplify_and_const_int (rtx x, scalar_int_mode
> > mode, rtx varop,
> >
> > /* Given a REG X of mode XMODE, compute which bits in X can be nonzero.
> > We don't care about bits outside of those defined in MODE.
> > + We DO care about all the bits in MODE, even if XMODE is smaller than
> > MODE.
> >
> > For most X this is simply GET_MODE_MASK (GET_MODE (MODE)), but if X is
> > a shift, AND, or zero_extract, we can do better. */
>
> I think this is okay for trunk, and for backports after waiting a week
> or so for fallout. Thanks!
Thanks, applied to trunk.
Jozef
Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
Hi!
On Fri, Dec 14, 2018 at 03:22:13PM +, Jozef Lawrynowicz wrote:
> 2018-12-14 Jozef Lawrynowicz
>
> gcc/ChangeLog:
> * combine.c (update_rsp_from_reg_equal): Only look for the nonzero bits
> of src in nonzero_bits_mode if the mode of src is MODE_INT and
> HWI_COMPUTABLE.
> (reg_nonzero_bits_for_combine): Add clarification to comment.
Is there some PR this fixes?
>
> diff --git a/gcc/combine.c b/gcc/combine.c
> index 7e61139..c93aaed 100644
> --- a/gcc/combine.c
> +++ b/gcc/combine.c
> @@ -1698,9 +1698,13 @@ update_rsp_from_reg_equal (reg_stat_type *rsp,
> rtx_insn *insn, const_rtx set,
>/* Don't call nonzero_bits if it cannot change anything. */
>if (rsp->nonzero_bits != HOST_WIDE_INT_M1U)
> {
> - bits = nonzero_bits (src, nonzero_bits_mode);
> + machine_mode mode = GET_MODE (x);
> + if (GET_MODE_CLASS (mode) == MODE_INT
> + && HWI_COMPUTABLE_MODE_P (mode))
> + mode = nonzero_bits_mode;
> + bits = nonzero_bits (src, mode);
>if (reg_equal && bits)
> - bits &= nonzero_bits (reg_equal, nonzero_bits_mode);
> + bits &= nonzero_bits (reg_equal, mode);
>rsp->nonzero_bits |= bits;
> }
>
> @@ -10224,6 +10228,7 @@ simplify_and_const_int (rtx x, scalar_int_mode mode,
> rtx varop,
>
> /* Given a REG X of mode XMODE, compute which bits in X can be nonzero.
> We don't care about bits outside of those defined in MODE.
> + We DO care about all the bits in MODE, even if XMODE is smaller than MODE.
>
> For most X this is simply GET_MODE_MASK (GET_MODE (MODE)), but if X is
> a shift, AND, or zero_extract, we can do better. */
I think this is okay for trunk, and for backports after waiting a week
or so for fallout. Thanks!
Segher
Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
Hi Segher,
Thanks for the review.
On Wed, 12 Dec 2018 19:47:53 -0600
Segher Boessenkool wrote:
> The unused bits in a MODE_PARTIAL_INT value are undefined, so nonzero_bits
> isn't valid for conversion in either direction.
>
> And *which* bits are undefined isn't defined anywhere either, so we cannot
> convert to/from smaller MODE_INT modes, either.
Can't we use the last_set_nonzero_bits if last_set_mode was MODE_INT and the
current mode is MODE_PARTIAL_INT? As long as the current mode bitsize is less
than the bitsize of nonzero_bits_mode, which it will be if we've gotten to this
point.
If the current mode is MODE_PARTIAL_INT, then on entry to
reg_nonzero_bits_for_combine, the current nonzero_bits has already been
initialized to GET_MODE_MASK(mode), so we are not at risk of disturbing the
undefined bits, as we are only ever doing &= with GET_MODE_MASK(mode).
However, the above suggestion doesn't actually provide any visible benefit to
testresults, so it doesn't need to be included.
> I don't see how that follows; not all bits in MODE_PARTIAL_INT modes
> are necessarily valid.
Yes, this was an oversight on my part.
nonzero_bits_mode is only used to calculate last_set_nonzero_bits if
last_set_mode is in the MODE_INT class.
If last_set_mode was MODE_PARTIAL_INT class, then last_set_mode was just that
partial int mode; it wasn't calculated in the wider nonzero_bits_mode.
After some further investigation, it seems we can attribute the recursion to
an inconsistency with how nonzero_bits() is invoked.
The mode passed to nonzero_bits(rtx, mode) is normally the mode of rtx
itself. But there are two cases where nonzero_bits_mode is used instead, even
if that is wider than the mode of the rtx.
In record_value_for_reg:
> rsp->last_set_mode = mode;
> if (GET_MODE_CLASS (mode) == MODE_INT
> && HWI_COMPUTABLE_MODE_P (mode))
> mode = nonzero_bits_mode;
> rsp->last_set_nonzero_bits = nonzero_bits (value, mode);
In update_rsp_from_reg_equal:
> if (rsp->nonzero_bits != HOST_WIDE_INT_M1U)
>{
> bits = nonzero_bits (src, nonzero_bits_mode);
Note that the the mode of src in update_rsp_from_reg_equal is not
checked to see if it is a MODE_INT class and HWI_COMPUTABLE, nonzero_bits_mode
is always used.
This mode passed to nonzero_bits() eventually makes its way to
reg_nonzero_bits_for_combine. rsp->last_set_mode is always the true mode of the
reg (i.e. not nonzero_bits_mode) from when it is set in record_value_for_reg.
So the recursion happens because update_rsp_from_reg_equal has asked for the
nonzero_bits in nonzero_bits_mode, but the last_set_mode was PSImode.
nonzero_bits_mode is not equal to PSImode, nor is it in the same class, so the
nonzero bits will never be reused.
So my revised patch (attached) instead modifies update_rsp_from_reg_equal to
only request nonzero_bits in nonzero_bits_mode if the mode class is MODE_INT
and HWI_COMPUTABLE. This gives it parity with how last_set_nonzero_bits are set
in record_value_for_reg.
I've regtested the attached patch for msp430-elf, currently bootstrapping and
testing on x86_64-pc-linux-gnu.
Is this ok for trunk if bootstrap and regtest for x86_64-pc-linux-gnu is
successful?
Jozef
2018-12-14 Jozef Lawrynowicz
gcc/ChangeLog:
* combine.c (update_rsp_from_reg_equal): Only look for the nonzero bits
of src in nonzero_bits_mode if the mode of src is MODE_INT and
HWI_COMPUTABLE.
(reg_nonzero_bits_for_combine): Add clarification to comment.
diff --git a/gcc/combine.c b/gcc/combine.c
index 7e61139..c93aaed 100644
--- a/gcc/combine.c
+++ b/gcc/combine.c
@@ -1698,9 +1698,13 @@ update_rsp_from_reg_equal (reg_stat_type *rsp, rtx_insn *insn, const_rtx set,
/* Don't call nonzero_bits if it cannot change anything. */
if (rsp->nonzero_bits != HOST_WIDE_INT_M1U)
{
- bits = nonzero_bits (src, nonzero_bits_mode);
+ machine_mode mode = GET_MODE (x);
+ if (GET_MODE_CLASS (mode) == MODE_INT
+ && HWI_COMPUTABLE_MODE_P (mode))
+ mode = nonzero_bits_mode;
+ bits = nonzero_bits (src, mode);
if (reg_equal && bits)
- bits &= nonzero_bits (reg_equal, nonzero_bits_mode);
+ bits &= nonzero_bits (reg_equal, mode);
rsp->nonzero_bits |= bits;
}
@@ -10224,6 +10228,7 @@ simplify_and_const_int (rtx x, scalar_int_mode mode, rtx varop,
/* Given a REG X of mode XMODE, compute which bits in X can be nonzero.
We don't care about bits outside of those defined in MODE.
+ We DO care about all the bits in MODE, even if XMODE is smaller than MODE.
For most X this is simply GET_MODE_MASK (GET_MODE (MODE)), but if X is
a shift, AND, or zero_extract, we can do better. */
Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
On Wed, Dec 12, 2018 at 12:09:19PM +, Jozef Lawrynowicz wrote: > Compilation of gcc.dg/pr85180.c and gcc.dg/pr87985.c times out after 5 minutes > for msp430 with -mlarge. > > nonzero_bits1 (from rtlanal.c), recurses many times for each reg > because reg_nonzero_bits_for_combine (combine.c) never considers using > last_set_nonzero_bits for the given reg when the reg is PSImode (i.e. Pmode > for > msp430-elf -mlarge). > > nonzero bits for a mode of class MODE_PARTIAL_INT are valid for a mode of > class > MODE_INT, and vice-versa. The unused bits in a MODE_PARTIAL_INT value are undefined, so nonzero_bits isn't valid for conversion in either direction. And *which* bits are undefined isn't defined anywhere either, so we cannot convert to/from smaller MODE_INT modes, either. > The existing comment in reg_nonzero_bits_for_combine > explaining why last_set_nonzero_bits is valid even when the precision of the > last set mode is less than the current mode, also explains why > MODE_PARTIAL_INT and MODE_INT can be used interchangeably here: > > > record_value_for_reg invoked nonzero_bits on the register > > with nonzero_bits_mode (because last_set_mode is necessarily integral > > and HWI_COMPUTABLE_MODE_P in this case) so bits in nonzero_bits_mode > > are all valid, hence in mode too since nonzero_bits_mode is defined > > to the largest HWI_COMPUTABLE_MODE_P mode. I don't see how that follows; not all bits in MODE_PARTIAL_INT modes are necessarily valid. Perhaps it is true that you can return whatever you want here, for the undefined bits in a partial int var; but you'll need to justify that then, it isn't obvious to me at least. > + && (GET_MODE_CLASS (mode) == MODE_INT > + || GET_MODE_CLASS (mode) == MODE_PARTIAL_INT))) That's SCALAR_INT_MODE_P fwiw. Thanks, Segher
[PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
Compilation of gcc.dg/pr85180.c and gcc.dg/pr87985.c times out after 5 minutes for msp430 with -mlarge. nonzero_bits1 (from rtlanal.c), recurses many times for each reg because reg_nonzero_bits_for_combine (combine.c) never considers using last_set_nonzero_bits for the given reg when the reg is PSImode (i.e. Pmode for msp430-elf -mlarge). nonzero bits for a mode of class MODE_PARTIAL_INT are valid for a mode of class MODE_INT, and vice-versa. The existing comment in reg_nonzero_bits_for_combine explaining why last_set_nonzero_bits is valid even when the precision of the last set mode is less than the current mode, also explains why MODE_PARTIAL_INT and MODE_INT can be used interchangeably here: > record_value_for_reg invoked nonzero_bits on the register > with nonzero_bits_mode (because last_set_mode is necessarily integral > and HWI_COMPUTABLE_MODE_P in this case) so bits in nonzero_bits_mode > are all valid, hence in mode too since nonzero_bits_mode is defined > to the largest HWI_COMPUTABLE_MODE_P mode. The attached patch fixes the timeout with mlarge (compilation takes only a couple of seconds) by allowing the last set nonzero bits for a reg to be used if either the current mode or last mode is MODE_PARTIAL_INT or MODE_INT. Currently only MODE_INT is considered. Successfully bootstrapped and regtested x86_64-pc-linux-gnu and msp430-elf -msmall and -mlarge. Ok for trunk? >From 753dbbfab665020cece59496765086b3debe23f9 Mon Sep 17 00:00:00 2001 From: Jozef Lawrynowicz Date: Tue, 27 Nov 2018 19:03:53 + Subject: [PATCH] Use last_set_nonzero_bits for a REG when REG mode is MODE_PARTIAL_INT 2018-12-12 Jozef Lawrynowicz gcc/ChangeLog: * combine.c (reg_nonzero_bits_for_combine): Use last_set_nonzero_bits for a reg if the current mode or last set mode was MODE_PARTIAL_INT. --- gcc/combine.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/gcc/combine.c b/gcc/combine.c index 7e61139..73b80b6 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -10245,8 +10245,10 @@ reg_nonzero_bits_for_combine (const_rtx x, scalar_int_mode xmode, if (rsp->last_set_value != 0 && (rsp->last_set_mode == mode || (REGNO (x) >= FIRST_PSEUDO_REGISTER - && GET_MODE_CLASS (rsp->last_set_mode) == MODE_INT - && GET_MODE_CLASS (mode) == MODE_INT)) + && (GET_MODE_CLASS (rsp->last_set_mode) == MODE_INT + || GET_MODE_CLASS (rsp->last_set_mode) == MODE_PARTIAL_INT) + && (GET_MODE_CLASS (mode) == MODE_INT + || GET_MODE_CLASS (mode) == MODE_PARTIAL_INT))) && ((rsp->last_set_label >= label_tick_ebb_start && rsp->last_set_label < label_tick) || (rsp->last_set_label == label_tick -- 2.7.4
