From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 117374 invoked by alias); 1 Sep 2017 08:51:30 -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 116970 invoked by uid 89); 1 Sep 2017 08:51:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.6 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=H*M:4e38 X-HELO: relay1.mentorg.com Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 01 Sep 2017 08:51:24 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-MBX-04.mgc.mentorg.com) by relay1.mentorg.com with esmtp id 1dnhfc-0002Sd-Us from Tom_deVries@mentor.com ; Fri, 01 Sep 2017 01:51:21 -0700 Received: from [127.0.0.1] (137.202.0.87) by SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) with Microsoft SMTP Server (TLS) id 15.0.1263.5; Fri, 1 Sep 2017 09:51:17 +0100 Subject: Re: [PATCH] Fix bug in simplify_ternary_operation To: Jeff Law CC: GCC Patches References: <82ea4bb9-15cb-b00d-c6af-e1de926a9cec@mentor.com> <27b0d8a8-935a-dd45-b0ff-2d86f8d854e0@redhat.com> From: Tom de Vries Message-ID: Date: Fri, 01 Sep 2017 08:51:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <27b0d8a8-935a-dd45-b0ff-2d86f8d854e0@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-ClientProxiedBy: svr-ies-mbx-01.mgc.mentorg.com (139.181.222.1) To SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) X-SW-Source: 2017-09/txt/msg00018.txt.bz2 On 08/31/2017 11:44 PM, Jeff Law wrote: > On 08/28/2017 12:26 PM, Tom de Vries wrote: >> Hi, >> >> I think I found a bug in r17465: >> ... >>> * cse.c (simplify_ternary_operation): Handle more IF_THEN_ELSE >>> simplifications. >>> >>> diff --git a/gcc/cse.c b/gcc/cse.c >>> index e001597..3c27387 100644 >>> --- a/gcc/cse.c >>> +++ b/gcc/cse.c >>> @@ -4713,6 +4713,17 @@ simplify_ternary_operation (code, mode, >>> op0_mode, op0, op1, op2) >> >> Note: the parameters of simplify_ternary_operation have the following >> meaning: >> ... >> /* Simplify CODE, an operation with result mode MODE and three operands, >> OP0, OP1, and OP2. OP0_MODE was the mode of OP0 before it became >> a constant. Return 0 if no simplifications is possible. */ >> >> rtx >> simplify_ternary_operation (code, mode, op0_mode, op0, op1, op2) >> enum rtx_code code; >> enum machine_mode mode, op0_mode; >> rtx op0, op1, op2; >> ... >> >>> && rtx_equal_p (XEXP (op0, 1), op1) >>> && rtx_equal_p (XEXP (op0, 0), op2)) >>> return op2; >>> + else if (! side_effects_p (op0)) >>> + { >>> + rtx temp; >>> + temp = simplify_relational_operation (GET_CODE (op0), op0_mode, >>> + XEXP (op0, 0), XEXP >>> (op0, 1)); >> >> We're handling code == IF_THEN_ELSE here, so op0 is the condition, op1 >> is the 'then expr' and op2 is the 'else expr'. >> >> The parameters of simplify_relational_operation have the following meaning: >> ... >> /* Like simplify_binary_operation except used for relational operators. >> MODE is the mode of the operands, not that of the result. If MODE >> is VOIDmode, both operands must also be VOIDmode and we compare the >> operands in "infinite precision". >> >> If no simplification is possible, this function returns zero. >> Otherwise, it returns either const_true_rtx or const0_rtx. */ >> >> rtx >> simplify_relational_operation (code, mode, op0, op1) >> enum rtx_code code; >> enum machine_mode mode; >> rtx op0, op1; >> ... >> >> The problem in the patch is that we use op0_mode argument for the mode >> parameter. The mode parameter of simplify_relational_operation needs to >> be the mode of the operands of the condition, while op0_mode is the mode >> of the condition. >> >> Patch below fixes this on current trunk. >> >> [ I found this by running into an ICE in >> gcc.c-torture/compile/pr28776-2.c for gcn target. I haven't been able to >> reproduce this with an upstream branch yet. ] >> >> OK for trunk if bootstrap and reg-test for x86_64 succeeds? > So clearly setting cmp_mode to op0_mode is wrong. But we also have to > make sure that if cmp_mode is VOIDmode that either XEXP (op0, 0) has a > non-void mode or that XEXP (op0, 1) has a non-void mode, otherwise we're > likely to abort down in simplify_const_relational_operation. > You're referring to this assert: ... /* Check if the given comparison (done in the given MODE) is actually a tautology or a contradiction. If the mode is VOID_mode, the comparison is done in "infinite precision". If no simplification is possible, this function returns zero. Otherwise, it returns either const_true_rtx or const0_rtx. */ rtx simplify_const_relational_operation (enum rtx_code code, machine_mode mode, rtx op0, rtx op1) { ... gcc_assert (mode != VOIDmode || (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode)); ... added by Honza: ... * simplify-rtx.c (simplify_relational_operation): Verify that mode == VOIDmode implies both operands to be VOIDmode. ... In other words, rewriting the assert in more readable form: ... #define BOOL_IMPLIES(a, b) (!(a) || (b)) gcc_assert (BOOL_IMPLIES (mode == VOIDmode, (GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode))); ... [ I'd be in favor of rewriting imply relations using a macro or some such, I find it easier to understand. ] Now, simplify_relational_operation starts like this: ... rtx simplify_relational_operation (enum rtx_code code, machine_mode mode, machine_mode cmp_mode, rtx op0, rtx op1) { rtx tem, trueop0, trueop1; if (cmp_mode == VOIDmode) cmp_mode = GET_MODE (op0); if (cmp_mode == VOIDmode) cmp_mode = GET_MODE (op1); tem = simplify_const_relational_operation (code, cmp_mode, op0, op1); ... AFAIU, the cmp_mode ifs ensure that the assert in simplify_const_relational_operation doesn't trigger. > ISTM a better fix is to return NULL_RTX if cmp_mode is VOIDmode and both > the sub-operations are VOIDmode as well. > I don't think we need that. simplify_const_relational_operation can handle the situation that mode == VOIDmode && GET_MODE (op0) == VOIDmode && GET_MODE (op1) == VOIDmode. Thanks, - Tom > Can you try that and verify that pr28776-2.c continues to work? > jeff >