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
Sergos
