Message ID  20181212120919.13955fb3@jozefAspireVN7793G 

State  New 
Headers  show 
Series 

Related  show 
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 > msp430elf mlarge). > > nonzero bits for a mode of class MODE_PARTIAL_INT are valid for a mode of class > MODE_INT, and viceversa. 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
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 msp430elf, currently bootstrapping and testing on x86_64pclinuxgnu. Is this ok for trunk if bootstrap and regtest for x86_64pclinuxgnu is successful? Jozef 20181214 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. */
Hi! On Fri, Dec 14, 2018 at 03:22:13PM +0000, Jozef Lawrynowicz wrote: > 20181214 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
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: > > 20181214 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
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 20181212 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