* Re: [PATCH i386] PR47698 no CMOV for volatile mem
@ 2011-10-27 19:47 Uros Bizjak
2011-10-28 8:49 ` Richard Guenther
0 siblings, 1 reply; 12+ messages in thread
From: Uros Bizjak @ 2011-10-27 19:47 UTC (permalink / raw)
To: gcc-patches; +Cc: Sergey Ostanevich, H.J. Lu, Richard Guenther
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.
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-27 19:47 [PATCH i386] PR47698 no CMOV for volatile mem Uros Bizjak @ 2011-10-28 8:49 ` Richard Guenther 2011-10-28 13:25 ` Sergey Ostanevich 0 siblings, 1 reply; 12+ messages in thread From: Richard Guenther @ 2011-10-28 8:49 UTC (permalink / raw) To: Uros Bizjak; +Cc: gcc-patches, Sergey Ostanevich, H.J. Lu 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-28 8:49 ` Richard Guenther @ 2011-10-28 13:25 ` Sergey Ostanevich 2011-10-28 13:39 ` Richard Guenther 2011-10-28 16:07 ` Richard Henderson 0 siblings, 2 replies; 12+ messages in thread From: Sergey Ostanevich @ 2011-10-28 13:25 UTC (permalink / raw) To: Richard Guenther; +Cc: Uros Bizjak, gcc-patches, H.J. Lu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-28 13:25 ` Sergey Ostanevich @ 2011-10-28 13:39 ` Richard Guenther 2011-10-28 15:49 ` Sergey Ostanevich 2011-10-28 16:07 ` Richard Henderson 1 sibling, 1 reply; 12+ messages in thread From: Richard Guenther @ 2011-10-28 13:39 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: Uros Bizjak, gcc-patches, H.J. Lu [-- 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-28 13:39 ` Richard Guenther @ 2011-10-28 15:49 ` Sergey Ostanevich 2011-10-28 21:55 ` Sergey Ostanevich 0 siblings, 1 reply; 12+ messages in thread From: Sergey Ostanevich @ 2011-10-28 15:49 UTC (permalink / raw) To: Richard Guenther; +Cc: Uros Bizjak, gcc-patches, H.J. Lu [-- 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; +} ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-28 15:49 ` Sergey Ostanevich @ 2011-10-28 21:55 ` Sergey Ostanevich 2011-11-02 11:55 ` Richard Guenther 0 siblings, 1 reply; 12+ messages in thread From: Sergey Ostanevich @ 2011-10-28 21:55 UTC (permalink / raw) To: Richard Guenther; +Cc: Uros Bizjak, gcc-patches, H.J. Lu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-28 21:55 ` Sergey Ostanevich @ 2011-11-02 11:55 ` Richard Guenther 2011-11-06 18:48 ` Sergey Ostanevich 0 siblings, 1 reply; 12+ messages in thread From: Richard Guenther @ 2011-11-02 11:55 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: Uros Bizjak, gcc-patches, H.J. Lu [-- 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-11-02 11:55 ` Richard Guenther @ 2011-11-06 18:48 ` Sergey Ostanevich 2011-11-07 21:00 ` Eric Botcazou 0 siblings, 1 reply; 12+ messages in thread From: Sergey Ostanevich @ 2011-11-06 18:48 UTC (permalink / raw) To: Richard Guenther; +Cc: Uros Bizjak, gcc-patches, H.J. Lu, kirill.yukhin [-- 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; +} ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-11-06 18:48 ` Sergey Ostanevich @ 2011-11-07 21:00 ` Eric Botcazou 0 siblings, 0 replies; 12+ messages in thread From: Eric Botcazou @ 2011-11-07 21:00 UTC (permalink / raw) To: Sergey Ostanevich Cc: gcc-patches, Richard Guenther, Uros Bizjak, H.J. Lu, kirill.yukhin > 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-28 13:25 ` Sergey Ostanevich 2011-10-28 13:39 ` Richard Guenther @ 2011-10-28 16:07 ` Richard Henderson 1 sibling, 0 replies; 12+ messages in thread From: Richard Henderson @ 2011-10-28 16:07 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: Richard Guenther, Uros Bizjak, gcc-patches, H.J. Lu 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~ ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH i386] PR47698 no CMOV for volatile mem @ 2011-10-27 13:04 Sergey Ostanevich 2011-10-28 1:44 ` Richard Henderson 0 siblings, 1 reply; 12+ messages in thread From: Sergey Ostanevich @ 2011-10-27 13:04 UTC (permalink / raw) To: gcc-patches; +Cc: H.J. Lu [-- 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; +} ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH i386] PR47698 no CMOV for volatile mem 2011-10-27 13:04 Sergey Ostanevich @ 2011-10-28 1:44 ` Richard Henderson 0 siblings, 0 replies; 12+ messages in thread From: Richard Henderson @ 2011-10-28 1:44 UTC (permalink / raw) To: Sergey Ostanevich; +Cc: gcc-patches, H.J. Lu 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~ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2011-11-07 20:57 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-10-27 19:47 [PATCH i386] PR47698 no CMOV for volatile mem Uros Bizjak 2011-10-28 8:49 ` Richard Guenther 2011-10-28 13:25 ` Sergey Ostanevich 2011-10-28 13:39 ` Richard Guenther 2011-10-28 15:49 ` Sergey Ostanevich 2011-10-28 21:55 ` Sergey Ostanevich 2011-11-02 11:55 ` Richard Guenther 2011-11-06 18:48 ` Sergey Ostanevich 2011-11-07 21:00 ` Eric Botcazou 2011-10-28 16:07 ` Richard Henderson -- strict thread matches above, loose matches on Subject: below -- 2011-10-27 13:04 Sergey Ostanevich 2011-10-28 1:44 ` Richard Henderson
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).