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

[Sergey Ostanevich <sergos dot gnu at gmail dot com>]

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: 47698.patch
Description: Binary data