On Fri, Oct 28, 2011 at 4:52 PM, Richard Guenther <rguent...@suse.de> wrote: > On Fri, 28 Oct 2011, Sergey Ostanevich wrote: > >> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguent...@suse.de> wrote: >> > On Thu, 27 Oct 2011, Uros Bizjak wrote: >> > >> >> Hello! >> >> >> >> > Here's a patch for PR47698, which is about CMOV should not be >> >> > generated for memory address marked as volatile. >> >> > Successfully bootstrapped and passed make check on >> >> > x86_64-unknown-linux-gnu. >> >> >> >> >> >> PR rtl-optimization/47698 >> >> * config/i386/i386.c (ix86_expand_int_movcc) prevent CMOV generation >> >> for volatile mem >> >> >> >> PR rtl-optimization/47698 >> >> * gcc.target/i386/47698.c: New test >> >> >> >> Please use punctuation marks and correct capitalization in ChangeLog >> >> entries. >> >> >> >> OTOH, do we want to fix this per-target, or in the middle-end? >> > >> > The middle-end pattern documentation does not say operands 2 and 3 >> > are not evaluated if they do not end up being stored, so a middle-end >> > fix is more appropriate. >> > >> > Richard. >> > >> >> I have two observations: >> >> - the code for CMOV is under #ifdef in the mddle-end, which is >> explicitly marked as "have to be removed" (ifcvt.c:1446) >> - I have no clear evidence all platforms that support conditional move >> have the same semantics that lead to the PR >> >> I think the best way to address both concerns is to implement code >> that relies on а new hookup "volatile-safe CMOV" that is false by >> default. > > I suppose it's never safe for all architectures that support > memory operands in the source operand. > > Richard.
ok, at least there should be no big problem of missing optimization
around volatile memory.
apparently the problem is here:
ifcvt.c:2539 there is a test for side effects of source (which is 'a'
in this case)
2539 if (! noce_operand_ok (a) || ! noce_operand_ok (b))
(gdb) p debug_rtx(a)
(mem/v/c/i:DI (symbol_ref:DI ("mmio") [flags 0x40] <var_decl
0x7ffff1339140 mmio>) [2 mmio+0 S8 A64])
but inside noce_operand_ok() there is a wrong order of tests:
2332 if (MEM_P (op))
2333 return ! side_effects_p (XEXP (op, 0));
2334
2335 if (side_effects_p (op))
2336 return FALSE;
2337
where XEXP removes the memory reference leaving just symbol reference,
that has no volatile attribute
#0 side_effects_p (x=0x7ffff149c660) at ../../gcc/rtlanal.c:2152
(gdb) p debug_rtx(x)
(symbol_ref:DI ("mmio") [flags 0x40] <var_decl 0x7ffff1339140 mmio>)
Is the following fix is Ok?
I'm testing it so far.
Sergos
diff --git a/gcc/ifcvt.c b/gcc/ifcvt.c
index 784e2e8..3b05c2a 100644
--- a/gcc/ifcvt.c
+++ b/gcc/ifcvt.c
@@ -2329,12 +2329,12 @@ noce_operand_ok (const_rtx op)
{
/* We special-case memories, so handle any of them with
no address side effects. */
- if (MEM_P (op))
- return ! side_effects_p (XEXP (op, 0));
-
if (side_effects_p (op))
return FALSE;
+ if (MEM_P (op))
+ return ! side_effects_p (XEXP (op, 0));
+
return ! may_trap_p (op);
}
diff --git a/gcc/testsuite/gcc.target/i386/47698.c
b/gcc/testsuite/gcc.target/i386/47698.c
new file mode 100644
index 0000000..2c75109
--- /dev/null
+++ b/gcc/testsuite/gcc.target/i386/47698.c
@@ -0,0 +1,10 @@
+/* { dg-options "-Os" } */
+/* { dg-final { scan-assembler-not "cmov" } } */
+
+extern volatile unsigned long mmio;
+unsigned long foo(int cond)
+{
+ if (cond)
+ return mmio;
+ return 0;
+}
47698.patch
Description: Binary data
