From: Jakub Jelinek <jakub@redhat.com>
To: Richard Biener <rguenther@suse.de>, Jeff Law <law@redhat.com>,
Eric Botcazou <ebotcazou@adacore.com>,
Uros Bizjak <ubizjak@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: [PATCH] Check for sp push/pop insns in reg_set_p (PR target/79430)
Date: Thu, 27 Apr 2017 08:04:00 -0000 [thread overview]
Message-ID: <20170427073250.GW1809@tucnak> (raw)
Hi!
As mentioned in the PR and can be seen on the testcase (too large for
testsuite, with lots of delta reduction I got 48KB *.f90 file still using
a dozen of modules), we miscompile it because we have mem(sp+64) memory
(what %st is loaded from) and are checking whether it is safe to move
earlier in the insn stream, and modified_between_p tells us it is, except
there is a stack pop instruction (i.e. sp autoinc).
And sp autoinc is apparently special in GCC:
/* There are no REG_INC notes for SP. */
/* Cannot handle auto inc of the stack. */
if (inc_reg == stack_pointer_rtx)
etc. - it is present even on targets that have AUTO_INC_DEC 0 (like
i?86/x86_64), don't have REG_INC notes etc.
reg_set_p currently has:
/* We can be passed an insn or part of one. If we are passed an insn,
check if a side-effect of the insn clobbers REG. */
if (INSN_P (insn)
&& (FIND_REG_INC_NOTE (insn, reg)
so it handles insns with REG_INC notes fine, but doesn't know about the
SP special case.
The following patch handles that, plus then undoes that in ix86_agi_dependent
where from what I understood we want the previous behavior - push, pop and
call modifications of SP don't cause AGI stalls for addresses that have
SP base (SP can't appear as index).
Not really sure about the == stack_pointer_rtx vs.
REG_P () && REGNO () == STACK_POINTER_REGNUM, there is lots of code that
just uses pointer comparisons and others that check REGNO, as an example
of the former e.g. push/pop_operand. So, is SP always shared, or can there
be other REGs with SP regno?
Other than the ix86_agi_dependent which in my stats was the single case
that hit this difference, I've seen it making a difference e.g. in ifcvt
decisions, but at least the cases I've debugged didn't end up in any code
generation changes. E.g. both x86_64 and i686 libstdc++.so.6 and
libgo.so.11 as the two largest shared libraries built during bootstrap
are identical without/with this patch (objdump -dr is identical that is).
While without the config/i386/i386.c changes there were tons of differences.
Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
2017-04-27 Jakub Jelinek <jakub@redhat.com>
PR target/79430
* rtlanal.c (reg_set_p): If reg is a stack_pointer_rtx, also
check for stack push/pop autoinc.
* config/i386/i386.c (ix86_agi_dependent): Return false
if the only reason why modified_in_p returned true is that
addr is SP based and set_insn is a push or pop.
--- gcc/rtlanal.c.jj 2017-04-26 12:11:04.019878187 +0200
+++ gcc/rtlanal.c 2017-04-26 17:48:14.131705330 +0200
@@ -1221,6 +1221,24 @@ reg_set_p (const_rtx reg, const_rtx insn
|| find_reg_fusage (insn, CLOBBER, reg)))))
return true;
+ /* There are no REG_INC notes for SP autoinc. */
+ if (reg == stack_pointer_rtx && INSN_P (insn))
+ {
+ subrtx_var_iterator::array_type array;
+ FOR_EACH_SUBRTX_VAR (iter, array, PATTERN (insn), NONCONST)
+ {
+ rtx mem = *iter;
+ if (mem
+ && MEM_P (mem)
+ && GET_RTX_CLASS (GET_CODE (XEXP (mem, 0))) == RTX_AUTOINC)
+ {
+ if (XEXP (XEXP (mem, 0), 0) == stack_pointer_rtx)
+ return true;
+ iter.skip_subrtxes ();
+ }
+ }
+ }
+
return set_of (reg, insn) != NULL_RTX;
}
--- gcc/config/i386/i386.c.jj 2017-04-26 17:48:01.108877052 +0200
+++ gcc/config/i386/i386.c 2017-04-26 17:50:44.890717389 +0200
@@ -29243,7 +29243,27 @@ ix86_agi_dependent (rtx_insn *set_insn,
if (MEM_P (recog_data.operand[i]))
{
rtx addr = XEXP (recog_data.operand[i], 0);
- return modified_in_p (addr, set_insn) != 0;
+ if (modified_in_p (addr, set_insn) != 0)
+ {
+ /* No AGI stall if SET_INSN is a push or pop and USE_INSN
+ has SP based memory (unless index reg is modified in a pop). */
+ rtx set = single_set (set_insn);
+ if (set
+ && (push_operand (SET_DEST (set), GET_MODE (SET_DEST (set)))
+ || pop_operand (SET_SRC (set), GET_MODE (SET_SRC (set)))))
+ {
+ struct ix86_address parts;
+ if (ix86_decompose_address (addr, &parts)
+ && REG_P (parts.base)
+ && REGNO (parts.base) == STACK_POINTER_REGNUM
+ && (parts.index == NULL_RTX
+ || MEM_P (SET_DEST (set))
+ || !modified_in_p (parts.index, set_insn)))
+ return false;
+ }
+ return true;
+ }
+ return false;
}
return false;
}
Jakub
next reply other threads:[~2017-04-27 7:32 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-04-27 8:04 Jakub Jelinek [this message]
2017-04-27 17:38 ` Jeff Law
2017-05-01 9:24 ` Uros Bizjak
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=20170427073250.GW1809@tucnak \
--to=jakub@redhat.com \
--cc=ebotcazou@adacore.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.com \
--cc=rguenther@suse.de \
--cc=ubizjak@gmail.com \
/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).