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.
