From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22428 invoked by alias); 14 Mar 2016 17:37:45 -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 22414 invoked by uid 89); 14 Mar 2016 17:37:45 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.8 required=5.0 tests=AWL,BAYES_50,KAM_LAZY_DOMAIN_SECURITY,RCVD_IN_DNSWL_NONE,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=describing, selschedirc, UD:sel-sched-ir.c, insn_t X-HELO: smtp.ispras.ru Received: from smtp.ispras.ru (HELO smtp.ispras.ru) (83.149.199.79) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 14 Mar 2016 17:37:35 +0000 Received: from [10.10.3.121] (unknown [83.149.199.91]) by smtp.ispras.ru (Postfix) with ESMTP id D56F420414; Mon, 14 Mar 2016 20:37:32 +0300 (MSK) Date: Mon, 14 Mar 2016 17:37:00 -0000 From: Alexander Monakov To: Andrey Belevantsev cc: GCC Patches Subject: Re: [03/05] Fix PR 66660 In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (LNX 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2016-03/txt/msg00783.txt.bz2 On Mon, 14 Mar 2016, Andrey Belevantsev wrote: > We speculate an insn in the PR but we do not make a check for it though we > should. The thing that broke this was the fix for PR 45472. In that pr, we > have moved a volatile insn too far up because we failed to merge the bits > describing its volatility when we have processed a control flow split. The > code to propagate the insn pattern with the insn merging was added when the > volatility of the two insns from the both split branches differ. However, the > volatility of the speculated insn and its original differ: the original insn > may trap while the speculated version may not. Thus, we replace a speculative > pattern with the original one per the PR 45472 fix for no reason. > > The patch for this problem just limits the original fix for PR 45472 to apply > for non-speculative insns only. There is no test as it is not so easy to > construct one -- we could count the number of speculation check in the > resulting assembly but there is no way to force speculation to happen. > > Ok for trunk? > > gcc/ > > 2016-03-14 Andrey Belevantsev > > PR target/66660 > * sel-sched-ir.c (merge_expr): Do not propagate trap bits into > speculative insns. I think this line doesn't capture the issue at hand well; the issue is not in propagating trap bits, but rather unintentionally dropping the speculative pattern, right? I'd be happier with something like "If the pattern is already speculative, keep it, and do not check trap bits". diff --git a/gcc/sel-sched-ir.c b/gcc/sel-sched-ir.c index e181cb9..ec59280 100644 --- a/gcc/sel-sched-ir.c +++ b/gcc/sel-sched-ir.c @@ -1871,12 +1871,12 @@ merge_expr (expr_t to, expr_t from, insn_t split_point) /* Make sure that speculative pattern is propagated into exprs that have non-speculative one. This will provide us with consistent speculative bits and speculative patterns inside expr. */ - if ((EXPR_SPEC_DONE_DS (from) != 0 - && EXPR_SPEC_DONE_DS (to) == 0) - /* Do likewise for volatile insns, so that we always retain - the may_trap_p bit on the resulting expression. */ - || (VINSN_MAY_TRAP_P (EXPR_VINSN (from)) - && !VINSN_MAY_TRAP_P (EXPR_VINSN (to)))) + if (EXPR_SPEC_DONE_DS (to) == 0 + && (EXPR_SPEC_DONE_DS (from) != 0 + /* Do likewise for volatile insns, so that we always retain + the may_trap_p bit on the resulting expression. */ + || (VINSN_MAY_TRAP_P (EXPR_VINSN (from)) + && !VINSN_MAY_TRAP_P (EXPR_VINSN (to))))) change_vinsn_in_expr (to, EXPR_VINSN (from)); The patch looks unusual in that it reshuffles code while keeping comments; it seems the upper comment matches the code better now, while the lower one could be improved to say that may_trap_p is deliberately ignored when 'to' is already speculated. Finally, I'd recommend to switch around the two VINSN_MAY_TRAP_P tests so that condition on 'to' is consistently checked prior to condition on 'from'. OK with those changes. Thanks. Alexander