From patchwork Fri Oct 28 15:25:32 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Sergey Ostanevich X-Patchwork-Id: 122425 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id 413781007DE for ; Sat, 29 Oct 2011 02:25:57 +1100 (EST) Received: (qmail 1838 invoked by alias); 28 Oct 2011 15:25:52 -0000 Received: (qmail 1790 invoked by uid 22791); 28 Oct 2011 15:25:50 -0000 X-SWARE-Spam-Status: No, hits=-1.2 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, TW_ZJ X-Spam-Check-By: sourceware.org Received: from mail-fx0-f47.google.com (HELO mail-fx0-f47.google.com) (209.85.161.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Fri, 28 Oct 2011 15:25:33 +0000 Received: by faas16 with SMTP id s16so4339882faa.20 for ; Fri, 28 Oct 2011 08:25:32 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.4.215 with SMTP id 23mr6828516fas.8.1319815532247; Fri, 28 Oct 2011 08:25:32 -0700 (PDT) Received: by 10.223.113.144 with HTTP; Fri, 28 Oct 2011 08:25:32 -0700 (PDT) In-Reply-To: References: Date: Fri, 28 Oct 2011 19:25:32 +0400 Message-ID: Subject: Re: [PATCH i386] PR47698 no CMOV for volatile mem From: Sergey Ostanevich To: Richard Guenther Cc: Uros Bizjak , gcc-patches@gcc.gnu.org, "H.J. Lu" X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org On Fri, Oct 28, 2011 at 4:52 PM, Richard Guenther wrote: > On Fri, 28 Oct 2011, Sergey Ostanevich wrote: > >> On Fri, Oct 28, 2011 at 12:16 PM, Richard Guenther 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] ) [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] ) 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; +}