From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 81427 invoked by alias); 20 Nov 2017 02:52:31 -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 81410 invoked by uid 89); 20 Nov 2017 02:52:30 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: =?ISO-8859-1?Q?No, score=-26.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,GIT_PATCH_1,GIT_PATCH_2,GIT_PATCH_3,KB_WAM_FROM_NAME_SINGLEWORD,SPF_HELO_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy=patience, wrong.=c2?= 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 ESMTP; Mon, 20 Nov 2017 02:52:28 +0000 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 484664E90A; Mon, 20 Nov 2017 02:52:27 +0000 (UTC) Received: from localhost.localdomain (ovpn-112-12.rdu2.redhat.com [10.10.112.12]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6CBFD5D75E; Mon, 20 Nov 2017 02:52:26 +0000 (UTC) Subject: Re: [PATCH] Fix bug in simplify_ternary_operation To: Tom de Vries Cc: GCC Patches References: <82ea4bb9-15cb-b00d-c6af-e1de926a9cec@mentor.com> <27b0d8a8-935a-dd45-b0ff-2d86f8d854e0@redhat.com> From: Jeff Law Message-ID: <65851dc9-e427-eb58-469a-6327ef55fe7d@redhat.com> Date: Mon, 20 Nov 2017 03:12: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: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2017-11/txt/msg01701.txt.bz2 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. Thanks again for your patience. jeff