| Message ID | CAGYS_TLvcXYhByrkG+nmFPrATBM1duLGs51VZO-UYH3FWEf5Pw@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
> 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.
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; +}