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.133.124]) by sourceware.org (Postfix) with ESMTPS id 338913858C33 for ; Wed, 19 Jul 2023 13:12:04 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 338913858C33 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1689772323; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:in-reply-to:in-reply-to: references:references; bh=MjgOkaf/+F89dQi52ptJI65IFBOhtpEdaMXDTohN+vM=; b=bGXg/LieoLm9WwkduQCAQZzXukYOZ2utxG9cJ6OZcvBqkA++uTJVMthqGu/iBxSZuf+4wV nrzwBQtjiw5DwXm63SBTQqnEKrKp74EfyE6axMGyDZAX9tunoNHMBk6frF4iEIH0L9yqqV w4uCJZlkDvmEMa1atvxkOQ3T/rpCwfo= Received: from mimecast-mx02.redhat.com (66.187.233.73 [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-440-Pu8oB8J6M2aJvSeW7_BNmQ-1; Wed, 19 Jul 2023 09:11:52 -0400 X-MC-Unique: Pu8oB8J6M2aJvSeW7_BNmQ-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 350EB280BCA1; Wed, 19 Jul 2023 13:11:36 +0000 (UTC) Received: from tucnak.zalov.cz (unknown [10.45.226.176]) by smtp.corp.redhat.com (Postfix) with ESMTPS id EE948F6CDF; Wed, 19 Jul 2023 13:11:35 +0000 (UTC) Received: from tucnak.zalov.cz (localhost [127.0.0.1]) by tucnak.zalov.cz (8.17.1/8.17.1) with ESMTPS id 36JDBXwf2596650 (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384 bits=256 verify=NOT); Wed, 19 Jul 2023 15:11:33 +0200 Received: (from jakub@localhost) by tucnak.zalov.cz (8.17.1/8.17.1/Submit) id 36JDBWah2596647; Wed, 19 Jul 2023 15:11:32 +0200 Date: Wed, 19 Jul 2023 15:11:32 +0200 From: Jakub Jelinek To: Richard Biener Cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] middle-end/61747 - conditional move expansion and constants Message-ID: Reply-To: Jakub Jelinek References: <20230718112545.BC5D413494@imap2.suse-dmz.suse.de> MIME-Version: 1.0 In-Reply-To: <20230718112545.BC5D413494@imap2.suse-dmz.suse.de> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 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=-9.7 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,GIT_PATCH_0,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, Jul 18, 2023 at 01:25:45PM +0200, Richard Biener wrote: > > PR middle-end/61747 > * internal-fn.cc (expand_vec_cond_optab_fn): When the > value operands are equal to the original comparison operands > preserve that equality by re-using the comparison expansion. > * optabs.cc (emit_conditional_move): When the value operands > are equal to the comparison operands and would be forced to > a register by prepare_cmp_insn do so earlier, preserving the > equality. > > * g++.target/i386/pr61747.C: New testcase. > --- > gcc/internal-fn.cc | 17 ++++++++-- > gcc/optabs.cc | 32 ++++++++++++++++++- > gcc/testsuite/g++.target/i386/pr61747.C | 42 +++++++++++++++++++++++++ > 3 files changed, 88 insertions(+), 3 deletions(-) > create mode 100644 gcc/testsuite/g++.target/i386/pr61747.C > > diff --git a/gcc/internal-fn.cc b/gcc/internal-fn.cc > index e698f0bffc7..c83c3921792 100644 > --- a/gcc/internal-fn.cc > +++ b/gcc/internal-fn.cc > @@ -3019,8 +3019,21 @@ expand_vec_cond_optab_fn (internal_fn, gcall *stmt, convert_optab optab) > icode = convert_optab_handler (optab, mode, cmp_op_mode); > rtx comparison > = vector_compare_rtx (VOIDmode, tcode, op0a, op0b, unsignedp, icode, 4); > - rtx rtx_op1 = expand_normal (op1); > - rtx rtx_op2 = expand_normal (op2); > + /* vector_compare_rtx legitimizes operands, preserve equality when > + expanding op1/op2. */ > + rtx rtx_op1, rtx_op2; > + if (operand_equal_p (op1, op0a)) > + rtx_op1 = XEXP (comparison, 0); > + else if (operand_equal_p (op1, op0b)) > + rtx_op1 = XEXP (comparison, 1); > + else > + rtx_op1 = expand_normal (op1); > + if (operand_equal_p (op2, op0a)) > + rtx_op2 = XEXP (comparison, 0); > + else if (operand_equal_p (op2, op0b)) > + rtx_op2 = XEXP (comparison, 1); > + else > + rtx_op2 = expand_normal (op2); > > rtx target = expand_expr (lhs, NULL_RTX, VOIDmode, EXPAND_WRITE); > create_output_operand (&ops[0], target, mode); The above LGTM, it relies on vector_compare_rtx not swapping the arguments or performing some other comparison canonicalization, but at least right now that function doesn't seem to do that. > --- a/gcc/optabs.cc > +++ b/gcc/optabs.cc > @@ -5119,13 +5119,43 @@ emit_conditional_move (rtx target, struct rtx_comparison comp, > last = get_last_insn (); > do_pending_stack_adjust (); > machine_mode cmpmode = comp.mode; > + rtx orig_op0 = XEXP (comparison, 0); > + rtx orig_op1 = XEXP (comparison, 1); > + rtx op2p = op2; > + rtx op3p = op3; > + /* If we are optimizing, force expensive constants into a register > + but preserve an eventual equality with op2/op3. */ > + if (CONSTANT_P (orig_op0) && optimize > + && (rtx_cost (orig_op0, mode, COMPARE, 0, > + optimize_insn_for_speed_p ()) > + > COSTS_N_INSNS (1)) > + && can_create_pseudo_p ()) > + { > + XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); > + if (rtx_equal_p (orig_op0, op2)) > + op2p = XEXP (comparison, 0); > + if (rtx_equal_p (orig_op0, op3)) > + op3p = XEXP (comparison, 0); > + } > + if (CONSTANT_P (orig_op1) && optimize > + && (rtx_cost (orig_op1, mode, COMPARE, 0, > + optimize_insn_for_speed_p ()) > + > COSTS_N_INSNS (1)) > + && can_create_pseudo_p ()) > + { > + XEXP (comparison, 1) = force_reg (cmpmode, orig_op1); > + if (rtx_equal_p (orig_op1, op2)) > + op2p = XEXP (comparison, 1); > + if (rtx_equal_p (orig_op1, op3)) > + op3p = XEXP (comparison, 1); > + } I'm worried here, because prepare_cmp_insn before doing almost identical forcing to reg does if (CONST_SCALAR_INT_P (y)) canonicalize_comparison (mode, &comparison, &y); which the above change will make not happen anymore (for the more expensive constants). If we have a match between at least one of the comparison operands and op2/op3, I think having equivalency there is perhaps more important than the canonicalization, but it would be nice not to break it even if there is no match. So, perhaps force_reg only if there is a match? force_reg (cmpmode, force_reg (cmpmode, x)) is equivalent to force_reg (cmpmode, x), so perhaps: { if (rtx_equal_p (orig_op0, op2)) op2p = XEXP (comparison, 0) = force_reg (cmpmode, orig_op0); if (rtx_equal_p (orig_op0, op3)) op3p = XEXP (comparison, 0) = force_reg (cmpmode, XEXP (comparison, 0)); } and similarly for the other body? Jakub