From: Kyrill Tkachov <kyrylo.tkachov@arm.com>
To: Bernd Schmidt <bschmidt@redhat.com>,
GCC Patches <gcc-patches@gcc.gnu.org>
Cc: Jeff Law <law@redhat.com>
Subject: Re: [PATCH][RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves
Date: Thu, 19 Nov 2015 10:28:00 -0000 [thread overview]
Message-ID: <564DA454.2080704@arm.com> (raw)
In-Reply-To: <564C40D1.80409@arm.com>
[-- Attachment #1: Type: text/plain, Size: 3290 bytes --]
On 18/11/15 09:11, Kyrill Tkachov wrote:
>
> On 17/11/15 23:11, Bernd Schmidt wrote:
>> On 11/17/2015 02:03 PM, Kyrill Tkachov wrote:
>>> + || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn))))
>>> return false;
>>
>>> Well, I think the statement we want to make is
>>> "return false from this function if the two expressions contain the same
>>> register number".
>>
>> I looked at it again and I think I'll need more time to really figure out what's going on in this pass.
>>
>> However, about the above... either I'm very confused, or the actual statement here is "return false _unless_ the two expressions contain the same register number". In the testcase, the regs in question are ax and bx, which is then
>> rejected if the patch has been applied.
>
> I'm sorry, it is my mistake in explaining. I meant to say:
> "return false from this function unless the two expressions contain the same
> register number"
>
>>
>> (gdb) p tmp_reg
>> (reg:SI 0 ax)
>> (gdb) p cand->insn
>> (insn 59 117 60 7 (set (reg:SI 4 si [orig:107 D.1813 ] [107])
>> (sign_extend:SI (reg:HI 3 bx [orig:99 D.1813 ] [99])))
>>
>> And I think it would be better to strengthen that to "return false unless registers are identical". (From the above it's clear however that a plain rtx_equal_p wouldn't work since there's an extension in the operand).
>
> So the three relevant instructions are:
> I1: (set (reg:HI 0 ax)
> (mem:HI (symbol_ref:DI ("f"))))
>
> I2: (set (reg:SI 3 bx)
> (if_then_else:SI (eq (reg:CCZ 17 flags)
> (const_int 0))
> (reg:SI 0 ax)
> (reg:SI 3 bx)))
>
> I3: (set (reg:SI 4 si)
> (sign_extend:SI (reg:HI 3 bx)))
>
> I1 is def_insn, I3 is cand->insn. tmp_reg is 'ax'. What we want to do is reject this transformation
> because the destination of def_insn (aka I1), that is 'ax', is not the operand of the extend operation
> in cand->insn (aka I3). As you said, rtx_equal won't work on just SET_SRC (PATTERN (cand->insn)) because
> it's an extend operation. So reg_overlap_mentioned should be appropriate.
> Hope this helps.
>
>>
>> Also, I had another look at the testcase. It uses __builtin_printf and dg-output, which is at least unusual. Try to use the normal "if (cond) abort ()".
>>
>
> I did not try to remove the __builtin_printf because the use of 'f' there is needed to trigger this.
> There are at least two other dups of this PR: 68328 and 68185 the testcases for which have an "if (cond) abort ();" form.
> I'll use them instead.
>
For completeness' sake here's the patch I'm proposing.
I've used the testcases from the other two PRs that exhibit the same problem and use
the if (cond) abort (); form.
Kyrill
2015-11-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68194
PR rtl-optimization/68328
PR rtl-optimization/68185
* ree.c (combine_reaching_defs): Reject copy_needed case if defining
insn does not feed directly into candidate extension insn.
2015-11-16 Kyrylo Tkachov <kyrylo.tkachov@arm.com>
PR rtl-optimization/68194
* gcc.c-torture/execute/pr68185.c: Likewise.
* gcc.c-torture/execute/pr68328.c: Likewise.
>
>>
>> Bernd
>>
>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: ree-fix.patch --]
[-- Type: text/x-patch; name=ree-fix.patch, Size: 3117 bytes --]
commit 7ca1b135babbe3a542c850591e1b0e8736a19f55
Author: Kyrylo Tkachov <kyrylo.tkachov@arm.com>
Date: Fri Nov 13 15:01:47 2015 +0000
[RTL-ree] PR rtl-optimization/68194: Restrict copy instruction in presence of conditional moves
diff --git a/gcc/ree.c b/gcc/ree.c
index b8436f2..e91d164 100644
--- a/gcc/ree.c
+++ b/gcc/ree.c
@@ -814,7 +814,30 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state)
rtx tmp_reg = gen_rtx_REG (GET_MODE (SET_DEST (PATTERN (cand->insn))),
REGNO (SET_DEST (*dest_sub_rtx)));
- if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn))))
+
+ /* When transforming:
+ (set (reg1) (expression))
+ ...
+ (set (reg2) (any_extend (reg1)))
+
+ into
+
+ (set (reg2) (any_extend (expression)))
+ (set (reg1) (reg2))
+ make sure that reg1 from the first set feeds directly into the extend.
+ This may not hold in a situation with an intermediate
+ conditional copy i.e.
+ I1: (set (reg3) (expression))
+ I2: (set (reg1) (cond ? reg3 : reg1))
+ I3: (set (reg2) (any_extend (reg1)))
+
+ where I3 is cand, I1 is def_insn and I2 is a conditional copy.
+ We want to avoid transforming that into:
+ (set (reg2) (any_extend (expression)))
+ (set (reg1) (reg2))
+ (set (reg1) (cond ? reg3 : reg1)). */
+ if (reg_overlap_mentioned_p (tmp_reg, SET_DEST (PATTERN (cand->insn)))
+ || !reg_overlap_mentioned_p (tmp_reg, SET_SRC (PATTERN (cand->insn))))
return false;
/* The destination register of the extension insn must not be
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68185.c b/gcc/testsuite/gcc.c-torture/execute/pr68185.c
new file mode 100644
index 0000000..826531b
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68185.c
@@ -0,0 +1,29 @@
+int a, b, d = 1, e, f, o, u, w = 1, z;
+short c, q, t;
+
+int
+main ()
+{
+ char g;
+ for (; d; d--)
+ {
+ while (o)
+ for (; e;)
+ {
+ c = b;
+ int h = o = z;
+ for (; u;)
+ for (; a;)
+ ;
+ }
+ if (t < 1)
+ g = w;
+ f = g;
+ g && (q = 1);
+ }
+
+ if (q != 1)
+ __builtin_abort ();
+
+ return 0;
+}
diff --git a/gcc/testsuite/gcc.c-torture/execute/pr68328.c b/gcc/testsuite/gcc.c-torture/execute/pr68328.c
new file mode 100644
index 0000000..edf244b
--- /dev/null
+++ b/gcc/testsuite/gcc.c-torture/execute/pr68328.c
@@ -0,0 +1,44 @@
+int a, b, c = 1, d = 1, e;
+
+__attribute__ ((noinline, noclone))
+ int foo (void)
+{
+ asm volatile ("":::"memory");
+ return 4195552;
+}
+
+__attribute__ ((noinline, noclone))
+ void bar (int x, int y)
+{
+ asm volatile (""::"g" (x), "g" (y):"memory");
+ if (y == 0)
+ __builtin_abort ();
+}
+
+int
+baz (int x)
+{
+ char g, h;
+ int i, j;
+
+ foo ();
+ for (;;)
+ {
+ if (c)
+ h = d;
+ g = h < x ? h : 0;
+ i = (signed char) ((unsigned char) (g - 120) ^ 1);
+ j = i > 97;
+ if (a - j)
+ bar (0x123456, 0);
+ if (!b)
+ return e;
+ }
+}
+
+int
+main ()
+{
+ baz (2);
+ return 0;
+}
next prev parent reply other threads:[~2015-11-19 10:28 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-11-16 14:08 Kyrill Tkachov
2015-11-16 18:41 ` Bernd Schmidt
2015-11-17 9:08 ` Kyrill Tkachov
2015-11-17 9:49 ` Kyrill Tkachov
2015-11-17 10:17 ` Kyrill Tkachov
2015-11-17 12:10 ` Bernd Schmidt
2015-11-17 13:03 ` Kyrill Tkachov
2015-11-17 23:11 ` Bernd Schmidt
2015-11-18 9:11 ` Kyrill Tkachov
2015-11-19 10:28 ` Kyrill Tkachov [this message]
2015-11-20 1:41 ` Bernd Schmidt
2015-11-20 9:16 ` Kyrill Tkachov
2015-11-23 15:12 ` Kyrill Tkachov
2015-11-24 13:33 ` Kyrill Tkachov
2015-11-24 13:42 ` Bernd Schmidt
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=564DA454.2080704@arm.com \
--to=kyrylo.tkachov@arm.com \
--cc=bschmidt@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=law@redhat.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).