[PATCH i386] PR47698 no CMOV for volatile mem

[PATCH i386] PR47698 no CMOV for volatile mem

[Sergey Ostanevich]

[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]

Hi!

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.

Is it Ok?

regards,
Sergos


/gcc

2011-10-27 Sergey Ostanevich

	PR rtl-optimization/47698
	* config/i386/i386.c (ix86_expand_int_movcc) prevent CMOV generation
	for volatile mem

/testsuite

2011-10-27 Sergey Ostanevich

	PR rtl-optimization/47698
	* gcc.target/i386/47698.c: New test


diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c6e09ae..afe5de3 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -18312,6 +18312,12 @@ ix86_expand_int_movcc (rtx operands[])
   rtx op0 = XEXP (operands[1], 0);
   rtx op1 = XEXP (operands[1], 1);

+  /* MOVCC semantics implies that source is always read which is wrong
+     for devices I/O that are defined using volatile in C. PR47698 */
+
+  if (MEM_P (operands[2]) && MEM_VOLATILE_P (operands[2]))
+    return false;
+
   start_sequence ();
   compare_op = ix86_expand_compare (code, op0, op1);
   compare_seq = get_insns ();
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;
+}

[-- Attachment #2: 47698.patch --]
[-- Type: application/octet-stream, Size: 1080 bytes --]

diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
index c6e09ae..afe5de3 100644
--- a/gcc/config/i386/i386.c
+++ b/gcc/config/i386/i386.c
@@ -18312,6 +18312,12 @@ ix86_expand_int_movcc (rtx operands[])
   rtx op0 = XEXP (operands[1], 0);
   rtx op1 = XEXP (operands[1], 1);

+  /* MOVCC semantics implies that source is always read which is wrong
+     for devices I/O that are defined using volatile in C. PR47698 */
+
+  if (MEM_P (operands[2]) && MEM_VOLATILE_P (operands[2]))
+    return false;
+
   start_sequence ();
   compare_op = ix86_expand_compare (code, op0, op1);
   compare_seq = get_insns ();
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;
+}

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Richard Henderson]

On 10/27/2011 05:17 AM, Sergey Ostanevich wrote:
> +  /* MOVCC semantics implies that source is always read which is wrong
> +     for devices I/O that are defined using volatile in C. PR47698 */
> +
> +  if (MEM_P (operands[2]) && MEM_VOLATILE_P (operands[2]))
> +    return false;

This looks to be at the wrong level.  This fix should be
tested in ifcvt.c where we attempted this.


r~

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Eric Botcazou]

>      2011-11-06 Sergey Ostanevich sergos.gnu@gmail.com
>
>      PR rtl-optimization/47698
>      * ifconv.c (noce_operand_ok): prevent CMOV generation
>      for volatile mem

There is no such file as ifconv.c though.  The first letter must be capitalized
and there must be a period at the end.  A more exact description would have 
been "Return false for mems with side effects".  The comment attached to the 
code should have been moved with the code.

>      2011-11-06 Sergey Ostanevich sergos.gnu@gmail.com
>
>      PR rtl-optimization/47698
>      * gcc.target/i386/47698.c: New test

Likewise.

-- 
Eric Botcazou

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Sergey Ostanevich]

[-- Attachment #1: Type: text/plain, Size: 4921 bytes --]

On Wed, Nov 2, 2011 at 3:42 PM, Richard Guenther <rguenther@suse.de> wrote:
> On Sat, 29 Oct 2011, Sergey Ostanevich wrote:
>
>> On Fri, Oct 28, 2011 at 7:25 PM, Sergey Ostanevich <sergos.gnu@gmail.com> wrote:
>> > On Fri, Oct 28, 2011 at 4:52 PM, Richard Guenther <rguenther@suse.de> wrote:
>> >> On Fri, 28 Oct 2011, Sergey Ostanevich wrote:
>> >>
>> >>> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguenther@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;
>> > +}
>> >
>>
>> bootstrapped and passed make check successfully on x86_64-unknown-linux-gnu
>
> Ok.
>
> Thanks,
> Richard.

Could someone please commit it for me?

Sergos

/gcc

     2011-11-06 Sergey Ostanevich sergos.gnu@gmail.com

     PR rtl-optimization/47698
     * ifconv.c (noce_operand_ok): prevent CMOV generation
     for volatile mem

/testsuites

     2011-11-06 Sergey Ostanevich sergos.gnu@gmail.com

     PR rtl-optimization/47698
     * gcc.target/i386/47698.c: New test

[-- Attachment #2: 47698.patch --]
[-- Type: application/octet-stream, Size: 934 bytes --]

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;
+}

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4330 bytes --]

On Sat, 29 Oct 2011, Sergey Ostanevich wrote:

> On Fri, Oct 28, 2011 at 7:25 PM, Sergey Ostanevich <sergos.gnu@gmail.com> wrote:
> > On Fri, Oct 28, 2011 at 4:52 PM, Richard Guenther <rguenther@suse.de> wrote:
> >> On Fri, 28 Oct 2011, Sergey Ostanevich wrote:
> >>
> >>> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguenther@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;
> > +}
> >
> 
> bootstrapped and passed make check successfully on x86_64-unknown-linux-gnu

Ok.

Thanks,
Richard.

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Sergey Ostanevich]

On Fri, Oct 28, 2011 at 7:25 PM, Sergey Ostanevich <sergos.gnu@gmail.com> wrote:
> On Fri, Oct 28, 2011 at 4:52 PM, Richard Guenther <rguenther@suse.de> wrote:
>> On Fri, 28 Oct 2011, Sergey Ostanevich wrote:
>>
>>> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguenther@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;
> +}
>

bootstrapped and passed make check successfully on x86_64-unknown-linux-gnu

Sergos

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Richard Henderson]

On 10/28/2011 05:49 AM, Sergey Ostanevich wrote:
> - 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

We're on solid ground here.  The arguments are assumed to always be
evaluated in RTL, *except* in the case of COND_EXEC.  Only true
predication can avoid an exception or side effects of touching memory.

> 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.

We do not need a new hook.


r~

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Sergey Ostanevich]

[-- Attachment #1: Type: text/plain, Size: 3625 bytes --]

On Fri, Oct 28, 2011 at 4:52 PM, Richard Guenther <rguenther@suse.de> wrote:
> On Fri, 28 Oct 2011, Sergey Ostanevich wrote:
>
>> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguenther@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;
+}

[-- Attachment #2: 47698.patch --]
[-- Type: application/octet-stream, Size: 934 bytes --]

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;
+}

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Richard Guenther]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1590 bytes --]

On Fri, 28 Oct 2011, Sergey Ostanevich wrote:

> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguenther@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.

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Sergey Ostanevich]

On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther <rguenther@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.

regards,
Sergos

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Richard Guenther]

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.

Re: [PATCH i386] PR47698 no CMOV for volatile mem

[Uros Bizjak]

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?

Uros.