Patchwork [i386] PR47698 no CMOV for volatile mem

login
register
mail settings
Submitter Sergey Ostanevich
Date Oct. 28, 2011, 3:25 p.m.
Message ID <CAGYS_TLvcXYhByrkG+nmFPrATBM1duLGs51VZO-UYH3FWEf5Pw@mail.gmail.com>
Download mbox | patch
Permalink /patch/122425/
State New
Headers show

Comments

Sergey Ostanevich - Oct. 28, 2011, 3:25 p.m.
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
Sergey Ostanevich - Oct. 28, 2011, 8:16 p.m.
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
Richard Guenther - Nov. 2, 2011, 11:42 a.m.
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.
Sergey Ostanevich - Nov. 6, 2011, 4:18 p.m.
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
Eric Botcazou - Nov. 7, 2011, 8:56 p.m.
>      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.

Patch

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