From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id AD1FE3858D28 for ; Wed, 3 Nov 2021 14:21:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org AD1FE3858D28 Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-25-3hwy1MkgMqiLM_jb9W0TCQ-1; Wed, 03 Nov 2021 10:21:22 -0400 X-MC-Unique: 3hwy1MkgMqiLM_jb9W0TCQ-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 099C919057AD; Wed, 3 Nov 2021 14:21:21 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.39.193.172]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 65ADB196F4; Wed, 3 Nov 2021 14:21:19 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.16.1/8.16.1) with ESMTPS id 1A3EKxfB2288983 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 3 Nov 2021 15:21:05 +0100 Received: (from jakub@localhost) by tucnak.zalov.cz (8.16.1/8.16.1/Submit) id 1A3EKjbP2288978; Wed, 3 Nov 2021 15:20:45 +0100 Date: Wed, 3 Nov 2021 15:20:45 +0100 From: Jakub Jelinek To: Tamar Christina Cc: Jonathan Wakely , Richard Biener , "gcc-patches@gcc.gnu.org" , nd Subject: Re: [PATCH] middle-end: fix de-optimizations with bitclear patterns on signed values Message-ID: <20211103142045.GO304296@tucnak> Reply-To: Jakub Jelinek References: <7s13204o-n6os-699-q544-65sr84rprnsq@fhfr.qr> <20211026132008.GY304296@tucnak> <20211026133625.GZ304296@tucnak> <20211026193953.GC304296@tucnak> MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=us-ascii Content-Disposition: inline X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 03 Nov 2021 14:21:27 -0000 On Wed, Nov 03, 2021 at 10:56:30AM +0000, Tamar Christina wrote: > The spaceship operator is looking for (res & 1) == res which was previously folded to (res & ~1) == 0. > This is now being folded further to ((unsigned) res) <= 1. Is that match.pd change already in the tree (which commit) or not yet? > --- a/gcc/tree-ssa-phiopt.c > +++ b/gcc/tree-ssa-phiopt.c > @@ -2038,6 +2038,26 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > gimple *orig_use_stmt = use_stmt; > tree orig_use_lhs = NULL_TREE; > int prec = TYPE_PRECISION (TREE_TYPE (phires)); > + > + /* Deal with if match.pd has rewritten the (res & ~1) == 0 > + into res <= 1 and has left a type-cast for signed types. */ The above sentence doesn't make sense gramatically. Either Deal with match.pd rewriting ... and leaving ... or Deal with the case when match.pd ... or something similar? > + if (gimple_assign_cast_p (use_stmt)) > + { > + orig_use_lhs = gimple_assign_lhs (use_stmt); > + /* Match.pd would have only done this for a signed type, > + so the conversion must be to an unsigned one. */ Even at the start of sentence it should be match.pd, the file isn't called Match.pd. > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > + return false; > + if (SSA_NAME_OCCURS_IN_ABNORMAL_PHI (orig_use_lhs)) > + return false; > + if (EDGE_COUNT (phi_bb->preds) != 4) > + return false; > + if (!TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs))) > + return false; You are testing !TYPE_UNSIGNED (TREE_TYPE (orig_use_lhs)) twice, did you mean to instead test that it is a conversion from signed to unsigned (i.e. test if (TYPE_UNSIGNED (TREE_TYPE (gimple_assign_rhs1 (use_stmt)))) return false; ? Also, shouldn't it also test that both types are integral and have the same precision? > + if (!single_imm_use (orig_use_lhs, &use_p, &use_stmt)) > + return false; > + } > + > if (is_gimple_assign (use_stmt) > && gimple_assign_rhs_code (use_stmt) == BIT_AND_EXPR > && TREE_CODE (gimple_assign_rhs2 (use_stmt)) == INTEGER_CST > @@ -2099,7 +2119,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > || !tree_fits_shwi_p (rhs) > || !IN_RANGE (tree_to_shwi (rhs), -1, 1)) > return false; > - if (orig_use_lhs) > + if (orig_use_lhs && !integer_onep (rhs)) This doesn't look safe. orig_use_lhs in this case means either that there was just a cast, or that there was BIT_AND_EXPR, or that were both, and you don't know which one it is. The decision shouldn't be done based on whether rhs is or isn't 1, but on whether there was the BIT_AND or not. > { > if ((cmp != EQ_EXPR && cmp != NE_EXPR) || !integer_zerop (rhs)) > return false; > @@ -2345,6 +2365,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? GE_EXPR : LE_EXPR; > else if (integer_minus_onep (rhs)) > res_cmp = one_cmp == LT_EXPR ? GT_EXPR : LT_EXPR; > + else if (integer_onep (rhs)) > + res_cmp = GE_EXPR; And this one should be guarded on either the cast present or the comparison done unsigned (so probably TYPE_UNSIGNED (TREE_TYPE (rhs)) && integer_onep (rhs))? > else > return false; > break; > @@ -2353,6 +2375,8 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > else if (integer_zerop (rhs)) > res_cmp = one_cmp; > + else if (integer_onep (rhs)) > + res_cmp = one_cmp; > else > return false; > break; Likewise. > @@ -2360,7 +2384,7 @@ spaceship_replacement (basic_block cond_bb, basic_block middle_bb, > if (integer_zerop (rhs)) > res_cmp = one_cmp == LT_EXPR ? LE_EXPR : GE_EXPR; > else if (integer_onep (rhs)) > - res_cmp = one_cmp; > + res_cmp = LE_EXPR; > else > return false; > break; Are you sure? Jakub