From: Jozef Lawrynowicz <jozef.l@mittosystems.com>
To: Segher Boessenkool <segher@kernel.crashing.org>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH] [MSP430] Fix gcc.dg/pr85180.c and gcc.dg/pr87985.c timeouts for msp430-elf -mlarge
Date: Fri, 14 Dec 2018 15:22:00 -0000 [thread overview]
Message-ID: <20181214152213.0e369052@jozef-Aspire-VN7-793G> (raw)
In-Reply-To: <20181213014752.GY3803@gate.crashing.org>
[-- Attachment #1: Type: text/plain, Size: 3230 bytes --]
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
[-- Attachment #2: 0-nonzerov2.diff --]
[-- Type: text/x-patch, Size: 1479 bytes --]
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,
\f
/* 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. */
next prev parent reply other threads:[~2018-12-14 15:22 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-12-12 12:09 Jozef Lawrynowicz
2018-12-13 1:48 ` Segher Boessenkool
2018-12-14 15:22 ` Jozef Lawrynowicz [this message]
2018-12-18 9:08 ` Segher Boessenkool
2018-12-18 10:35 ` Jozef Lawrynowicz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20181214152213.0e369052@jozef-Aspire-VN7-793G \
--to=jozef.l@mittosystems.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=segher@kernel.crashing.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).