* [PATCH] Don't expand MEM1 op= MEM2 as temp = MEM1; temp op= MEM2; MEM1 = temp just because we couldn't add REG_EQUIV note (PR rtl-optimization/56151)
@ 2013-02-11 19:49 Jakub Jelinek
2013-02-11 20:18 ` Jeff Law
0 siblings, 1 reply; 2+ messages in thread
From: Jakub Jelinek @ 2013-02-11 19:49 UTC (permalink / raw)
To: gcc-patches
Hi!
As discussed in this PR, MEM1 op= MEM2 is usually better expanded
as temp = MEM2; MEM1 op= temp; if target supports that, even when it
means we can't add a REG_EQUIV note in that case (it would be
self-referential).
Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok
for trunk?
2013-02-11 Jakub Jelinek <jakub@redhat.com>
Steven Bosscher <steven@gcc.gnu.org>
PR rtl-optimization/56151
* optabs.c (add_equal_note): Don't return 0 if target is a MEM,
equal to op0 or op1, and last_insn pattern is CODE operation
with MEM dest and one of the operands matches that MEM.
* gcc.target/i386/pr56151.c: New test.
--- gcc/optabs.c.jj 2013-01-16 08:30:10.000000000 +0100
+++ gcc/optabs.c 2013-02-11 15:28:16.543839881 +0100
@@ -190,17 +190,34 @@ add_equal_note (rtx insns, rtx target, e
if (GET_CODE (target) == ZERO_EXTRACT)
return 1;
- /* If TARGET is in OP0 or OP1, punt. We'd end up with a note referencing
- a value changing in the insn, so the note would be invalid for CSE. */
- if (reg_overlap_mentioned_p (target, op0)
- || (op1 && reg_overlap_mentioned_p (target, op1)))
- return 0;
-
for (last_insn = insns;
NEXT_INSN (last_insn) != NULL_RTX;
last_insn = NEXT_INSN (last_insn))
;
+ /* If TARGET is in OP0 or OP1, punt. We'd end up with a note referencing
+ a value changing in the insn, so the note would be invalid for CSE. */
+ if (reg_overlap_mentioned_p (target, op0)
+ || (op1 && reg_overlap_mentioned_p (target, op1)))
+ {
+ if (MEM_P (target)
+ && (rtx_equal_p (target, op0)
+ || (op1 && rtx_equal_p (target, op1))))
+ {
+ /* For MEM target, with MEM = MEM op X, prefer no REG_EQUAL note
+ over expanding it as temp = MEM op X, MEM = temp. See PR56151. */
+ set = single_set (last_insn);
+ if (set
+ && GET_CODE (SET_SRC (set)) == code
+ && MEM_P (SET_DEST (set))
+ && (rtx_equal_p (SET_DEST (set), XEXP (SET_SRC (set), 0))
+ || (op1 && rtx_equal_p (SET_DEST (set),
+ XEXP (SET_SRC (set), 1)))))
+ return 1;
+ }
+ return 0;
+ }
+
set = single_set (last_insn);
if (set == NULL_RTX)
return 1;
--- gcc/testsuite/gcc.target/i386/pr56151.c.jj 2013-02-11 16:20:51.459752951 +0100
+++ gcc/testsuite/gcc.target/i386/pr56151.c 2013-02-11 16:23:10.590964710 +0100
@@ -0,0 +1,17 @@
+/* PR rtl-optimization/56151 */
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+int vara, varb;
+
+void
+foo (int i, int j)
+{
+ vara = varb | vara;
+}
+
+/* Verify the above is compiled into movl varb, %reg; orl %reg, vara instead
+ of longer movl vara, %reg; orl varb, %reg; movl %reg, vara. */
+/* { dg-final { scan-assembler-not "mov\[^\n\r]*vara" { target nonpic } } } */
+/* { dg-final { scan-assembler-times "mov\[^\n\r]*varb" 1 { target nonpic } } } */
+/* { dg-final { scan-assembler-times "or\[^\n\r]*vara" 1 { target nonpic } } } */
Jakub
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] Don't expand MEM1 op= MEM2 as temp = MEM1; temp op= MEM2; MEM1 = temp just because we couldn't add REG_EQUIV note (PR rtl-optimization/56151)
2013-02-11 19:49 [PATCH] Don't expand MEM1 op= MEM2 as temp = MEM1; temp op= MEM2; MEM1 = temp just because we couldn't add REG_EQUIV note (PR rtl-optimization/56151) Jakub Jelinek
@ 2013-02-11 20:18 ` Jeff Law
0 siblings, 0 replies; 2+ messages in thread
From: Jeff Law @ 2013-02-11 20:18 UTC (permalink / raw)
To: Jakub Jelinek; +Cc: gcc-patches
On 02/11/13 12:49, Jakub Jelinek wrote:
> Hi!
>
> As discussed in this PR, MEM1 op= MEM2 is usually better expanded
> as temp = MEM2; MEM1 op= temp; if target supports that, even when it
> means we can't add a REG_EQUIV note in that case (it would be
> self-referential).
>
> Fixed thusly, bootstrapped/regtested on x86_64-linux and i686-linux, ok
> for trunk?
>
> 2013-02-11 Jakub Jelinek <jakub@redhat.com>
> Steven Bosscher <steven@gcc.gnu.org>
>
> PR rtl-optimization/56151
> * optabs.c (add_equal_note): Don't return 0 if target is a MEM,
> equal to op0 or op1, and last_insn pattern is CODE operation
> with MEM dest and one of the operands matches that MEM.
>
> * gcc.target/i386/pr56151.c: New test.
Note that expanding as MEM op= X puts the decision about whether or not
to use a temporary entirely into the backend, where it can be split via
a define_insn_and_split. I'm generally OK with that.
However, it is worth noting this can inhibit CSE if the MEM was already
available in a pseudo. I don't think this is serious enough to warrant
rejecting this change. If it turns out to be a problem we'll need to do
more tuning.
> + {
> + /* For MEM target, with MEM = MEM op X, prefer no REG_EQUAL note
> + over expanding it as temp = MEM op X, MEM = temp. See PR56151. */
I'd suggest adding a comment here about the details of 56151 rather than
referencing the BZ database. Ideally we want a developer to be able to
read the code and understand why it works the way it does. Now the
developer has to go find the bug in the database and read that too.
I'd also suggest a quick note that this style of code generation can
inhibit CSE in some cases.
With those two comment changes, approved.
jeff
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2013-02-11 20:18 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-02-11 19:49 [PATCH] Don't expand MEM1 op= MEM2 as temp = MEM1; temp op= MEM2; MEM1 = temp just because we couldn't add REG_EQUIV note (PR rtl-optimization/56151) Jakub Jelinek
2013-02-11 20:18 ` Jeff Law
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).