From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 95242 invoked by alias); 20 Nov 2015 14:09:05 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 95204 invoked by uid 89); 20 Nov 2015 14:09:04 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 20 Nov 2015 14:09:03 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id C4A7EB891C; Fri, 20 Nov 2015 14:09:01 +0000 (UTC) Received: from localhost.localdomain (vpn1-5-212.ams2.redhat.com [10.36.5.212]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tAKE8xcs030650; Fri, 20 Nov 2015 09:09:00 -0500 Subject: Re: Remove noce_mem_write_may_trap_or_fault_p in ifcvt To: Jeff Law , GCC Patches , Jakub Jelinek , Sebastian Pop , Michael Matz References: <563CE17F.6090308@t-online.de> <563CF3F0.8010703@redhat.com> <563CF504.2070604@redhat.com> <563CFD92.7020808@redhat.com> <563CFFC0.6020908@redhat.com> <563D170B.3010800@redhat.com> <564CCE73.1090907@redhat.com> <564D0E7C.9070703@redhat.com> From: Bernd Schmidt Message-ID: <564F297B.8040603@redhat.com> Date: Fri, 20 Nov 2015 14:09:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <564D0E7C.9070703@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-11/txt/msg02493.txt.bz2 On 11/19/2015 12:49 AM, Jeff Law wrote: > On 11/18/2015 12:16 PM, Bernd Schmidt wrote: >> I don't think so, actually. One safe option would be to rip it out and >> just stop transforming this case, but let's start by looking at the code >> just a bit further down, calling noce_can_store_speculate. This was >> added later than the code we're discussing, and it tries to verify that >> the same memory location will unconditionally be written to at a point >> later than the one we're trying to convert > And if we dig into that thread, Ian suggests this isn't terribly > important anyway. > > However, if we go even further upthread, we find an assertion from > Michael that this is critical for 456.hmmer and references BZ 27313. Cc'd in case he has additional input. > https://gcc.gnu.org/ml/gcc-patches/2007-08/msg01978.html > > Sadly, no testcase was included. BZ27313 is marked as fixed by the introduction of the tree cselim pass, thus the problem won't even be seen at RTL level. I'm undecided on whether cs-elim is safe wrt the store speculation vs locks concerns raised in the thread discussing Ian's noce_can_store_speculate_p, but that's not something we have to consider to solve the problem at hand. > So if it weren't for the assertion that it's critical for hmmr, I'd be > convinced that just ripping out was the right thing to do. > > Can you look at 27313 and hmmr and see if there's an impact. Just maybe > the critical stuff for those is handled by the tree if converter and we > can just rip out the clearly incorrect RTL bits without regressing > anything performance-wise. If there is an impact, then I think we have > to look at either improving the tree bits (so we can remove the rtl > bits) or we have to do real dataflow analysis in the rtl bits. So I made this change: if (!set_b && MEM_P (orig_x)) - { - /* Disallow the "if (...) x = a;" form (implicit "else x = x;") - for optimizations if writing to x may trap or fault, - i.e. it's a memory other than a static var or a stack slot, - is misaligned on strict aligned machines or is read-only. If - x is a read-only memory, then the program is valid only if we - avoid the store into it. If there are stores on both the - THEN and ELSE arms, then we can go ahead with the conversion; - either the program is broken, or the condition is always - false such that the other memory is selected. */ - if (noce_mem_write_may_trap_or_fault_p (orig_x)) - return FALSE; - - /* Avoid store speculation: given "if (...) x = a" where x is a - MEM, we only want to do the store if x is always set - somewhere in the function. This avoids cases like - if (pthread_mutex_trylock(mutex)) - ++global_variable; - where we only want global_variable to be changed if the mutex - is held. FIXME: This should ideally be expressed directly in - RTL somehow. */ - if (!noce_can_store_speculate_p (test_bb, orig_x)) - return FALSE; - } + return FALSE; As far as I can tell hmmer and the 27313 testcase are unaffected at -O2 (if anything, hmmer was very slightly faster afterwards). The run wasn't super-scientific, but I wouldn't have expected anything else given the existence of cs-elim. Ok to do the above, removing all the bits made unnecessary (including memory_must_be_modified_in_insn_p in alias.c)? Bernd