[MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge

Message ID 20181212120919.13955fb3@jozef-Aspire-VN7-793G
State New
Headers show
Series
  • [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
Related show

Commit Message

Jozef Lawrynowicz Dec. 12, 2018, 12:09 p.m.
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?

Comments

Segher Boessenkool Dec. 13, 2018, 1:47 a.m. | #1
On Wed, Dec 12, 2018 at 12:09:19PM +0000, 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
Jozef Lawrynowicz Dec. 14, 2018, 3:22 p.m. | #2
Hi Segher,

Thanks for the review.

On Wed, 12 Dec 2018 19:47:53 -0600
Segher Boessenkool <segher@kernel.crashing.org> 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  <jozef.l@mittosystems.com>

	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.  */
Segher Boessenkool Dec. 18, 2018, 9:08 a.m. | #3
Hi!

On Fri, Dec 14, 2018 at 03:22:13PM +0000, Jozef Lawrynowicz wrote:
> 2018-12-14  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

> 

> 	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
Jozef Lawrynowicz Dec. 18, 2018, 10:35 a.m. | #4
On Tue, 18 Dec 2018 03:08:51 -0600
Segher Boessenkool <segher@kernel.crashing.org> wrote:

> Hi!

> 

> On Fri, Dec 14, 2018 at 03:22:13PM +0000, Jozef Lawrynowicz wrote:

> > 2018-12-14  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

> > 

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

Patch

From 753dbbfab665020cece59496765086b3debe23f9 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
Date: Tue, 27 Nov 2018 19:03:53 +0000
Subject: [PATCH] Use last_set_nonzero_bits for a REG when REG mode is
 MODE_PARTIAL_INT

2018-12-12  Jozef Lawrynowicz  <jozef.l@mittosystems.com>

	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