On Wed, Nov 2, 2011 at 3:42 PM, Richard Guenther <rguent...@suse.de> wrote:
> On Sat, 29 Oct 2011, Sergey Ostanevich wrote:
>
>> On Fri, Oct 28, 2011 at 7:25 PM, Sergey Ostanevich <sergos....@gmail.com>
>> wrote:
>> > 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;
>> > +}
>> >
>>
>> 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....@gmail.com
PR rtl-optimization/47698
* ifconv.c (noce_operand_ok): prevent CMOV generation
for volatile mem
/testsuites
2011-11-06 Sergey Ostanevich sergos....@gmail.com
PR rtl-optimization/47698
* gcc.target/i386/47698.c: New test
47698.patch
Description: Binary data
