This is the mail archive of the
`gcc-patches@gcc.gnu.org`
mailing list for the GCC project.

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |

Other format: | [Raw text] |

*From*: Jozef Lawrynowicz <jozef dot l at mittosystems dot com>*To*: Segher Boessenkool <segher at kernel dot crashing dot org>*Cc*: gcc-patches at gcc dot gnu dot org*Date*: Fri, 14 Dec 2018 15:22:13 +0000*Subject*: Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge*References*: <20181212120919.13955fb3@jozef-Aspire-VN7-793G> <20181213014752.GY3803@gate.crashing.org>

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. */

**Follow-Ups**:**Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge***From:*Segher Boessenkool

**References**:**[PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge***From:*Jozef Lawrynowicz

**Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge***From:*Segher Boessenkool

Index Nav: | [Date Index] [Subject Index] [Author Index] [Thread Index] | |
---|---|---|

Message Nav: | [Date Prev] [Date Next] | [Thread Prev] [Thread Next] |