From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 61884 invoked by alias); 20 Nov 2017 08:41:38 -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 61299 invoked by uid 89); 20 Nov 2017 08:40:18 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-24.5 required=5.0 tests=AWL,BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy= 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; Mon, 20 Nov 2017 08:40:16 +0000 Received: from nat-ies.mentorg.com ([192.94.31.2] helo=SVR-IES-MBX-04.mgc.mentorg.com) by relay1.mentorg.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-SHA384:256) id 1eGhcj-0004U6-2o from Tom_deVries@mentor.com ; Mon, 20 Nov 2017 00:40:13 -0800 Received: from [172.30.72.34] (137.202.0.87) by SVR-IES-MBX-04.mgc.mentorg.com (139.181.222.4) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Mon, 20 Nov 2017 08:40:09 +0000 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> <65851dc9-e427-eb58-469a-6327ef55fe7d@redhat.com> From: Tom de Vries Message-ID: Date: Mon, 20 Nov 2017 09:48:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <65851dc9-e427-eb58-469a-6327ef55fe7d@redhat.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit 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-11/txt/msg01715.txt.bz2 On 11/20/2017 03:52 AM, Jeff Law wrote: > Sorry, it's taken so long to get back to this patch... > > > On 09/01/2017 02:51 AM, Tom de Vries wrote: >> 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. > I think you're right -- looking back at it again I think I mis-read the > assert. > > Go ahead and commit your change. Done. > Thanks again for your patience. > No problem, and thanks for the review :) . - Tom