From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pl1-x62b.google.com (mail-pl1-x62b.google.com [IPv6:2607:f8b0:4864:20::62b]) by sourceware.org (Postfix) with ESMTPS id 25D7F3858D3C for ; Wed, 29 Nov 2023 05:26:43 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 25D7F3858D3C Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 25D7F3858D3C Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::62b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701235605; cv=none; b=qOxrkD8Qvh5bN9BaLkNvf1KB0jZPIWTQECWtP/oxN18Bwe70azHDm8jiBT8DawuD1bGNhA9IKYDb+VMDhF115NSjWnkgEi3vY5h2bhQDxGUwPSRh4m18dkZJnDQjvLP1liwiUiK6gsH/LDpKOuaatLd5FLgoBSdSM1WW6McRtIY= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1701235605; c=relaxed/simple; bh=1N3VF2TxMNnZLmDk2zT5eYEeHoBFqmX7Z5+Yrzydgr4=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=odmH8DmktAj1x9QTZh1NHAh4EwKDxWaDEiU5NeZyxHl28vqdnx0EL1nYpozeIsQJ8K8LyUtCZt19H/jKMOKBqRZyehdWlKhlHwKeHSEyDWLdt5dxFIthQP0D4c1/4sGa9sIJ1PkB15EaNZOKhM5k83PzXV6SZ7FwweS7ovUnGhE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-pl1-x62b.google.com with SMTP id d9443c01a7336-1cf98ffc257so48281825ad.3 for ; Tue, 28 Nov 2023 21:26:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701235602; x=1701840402; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=Wsbk2QuLhk+NMECMUFEbB8xArkuCWG+49HlwyL38vP0=; b=VyQ2q4mkWtkzGLPA/KSu0aA6IswcfWb6xX0rqTIrVWqUEFOeer28FEcX6Mgq+M1KDX ES9W166xb7OiCk+2yF2H1euaBsUSDwrDsGDACVAH0SwIGkcXODH8hLg5rMk1laRXeaoO Q66O5AIQUuRjTllOyVwv61nPr/Z8dj+iCKXbXWAuJJfpmR2kqRRUhGY9eJL2ulClhZpA NufyOesVbsg+D0gme+vpJVo8Z+7Xm++gjCROzjSBUsqLxSs40bvR+tQERZdr/eipN8yG k+8sd6GTJ5HpZBe4YSlSZSWBPB8mFjSpDdDm53o1UJt/6mqHEWwrV+HPGRXElJDEdu+V tdOQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701235602; x=1701840402; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=Wsbk2QuLhk+NMECMUFEbB8xArkuCWG+49HlwyL38vP0=; b=uyH9QAugviWgNNgkDo/ThYmtQLTbVF6Sn5wCyhvWp9wHRFLMn6eFHRQZ683idtC2vu m2f1Ev6u/XAdZ3hya1DZjyNpdiiOydFUqGYTU+kMW8rNa0uRaYBsvA9Pt44z+70hjAhK UNC1jyapAX5jtrWR8q6qt0v3Q9d2+TwwyhEvcQsOk9es90D58FdL+WUhVpcxvX0Igjus X3/RAJUdCoVHgJuBVNaXjcsbrC46KjB3xJhTQGJSXVX6w1dn/MhlXA1ZJ0dzVpiMK3By oa3vkKbCWMXF1uO5lE2oYi7jqBo8dZH9mHMMrZdC1MHHPB6ftJKZGt30AyMdjszHNymS p3fQ== X-Gm-Message-State: AOJu0YwZ9o3fUXSIAi9Fg0/2qz1Am2NegSU6xPZEqNYnu+Aoo4Rn2rYw BVdF5slenQZQNcWZ+y4THpM= X-Google-Smtp-Source: AGHT+IE3W7RRM2k7qATow4jKiIWHSFroOayq6ELdlwo+IrRJYIUwzIlXFxIxo+r3DaMnx1VWHC/wyQ== X-Received: by 2002:a17:90a:198:b0:285:910a:98b4 with SMTP id 24-20020a17090a019800b00285910a98b4mr15377786pjc.24.1701235601672; Tue, 28 Nov 2023 21:26:41 -0800 (PST) Received: from [172.31.0.109] ([136.36.130.248]) by smtp.gmail.com with ESMTPSA id mm24-20020a17090b359800b00262eb0d141esm415304pjb.28.2023.11.28.21.26.38 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 28 Nov 2023 21:26:41 -0800 (PST) Message-ID: <27e492b4-feb9-4e80-8911-cc79d1d874b3@gmail.com> Date: Tue, 28 Nov 2023 22:26:35 -0700 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 2/4] [ifcvt] optimize x=c ? (y op z) : y by RISC-V Zicond like insns Content-Language: en-US To: Fei Gao , gcc-patches@gcc.gnu.org Cc: kito.cheng@gmail.com, palmer@dabbelt.com, zengxiao@eswincomputing.com References: <20231128023227.36200-1-gaofei@eswincomputing.com> <20231128023227.36200-2-gaofei@eswincomputing.com> From: Jeff Law In-Reply-To: <20231128023227.36200-2-gaofei@eswincomputing.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-8.3 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,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 11/27/23 19:32, Fei Gao wrote: > op=[PLUS, MINUS, IOR, XOR, ASHIFT, ASHIFTRT, LSHIFTRT, ROTATE, ROTATERT] > > SIGN_EXTEND, ZERO_EXTEND and SUBREG has been considered > to support SImode in 64-bit machine. Let's defer these for now. We're supposed to be wrapping up work that was posted before stage1 closed. If these opcodes were trivial to support, then I would let them through, but SUBREGs for example can be problematical as their semantics can be complex. > > Conditional op, if zero > rd = (rc == 0) ? (rs1 op rs2) : rs1 > --> > czero.nez rd, rs2, rc > op rd, rs1, rd > > Conditional op, if non-zero > rd = (rc != 0) ? (rs1 op rs2) : rs1 > --> > czero.eqz rd, rs2, rc > op rd, rs1, rd > > Co-authored-by: Xiao Zeng > > gcc/ChangeLog: > > * ifcvt.cc (noce_try_cond_zero_arith):handler for condtional zero based ifcvt > (noce_emit_czero): helper for noce_try_cond_zero_arith > (noce_cond_zero_binary_op_supported): check supported OPs for condtional zero based ifcvt > (get_base_reg): get base reg of a subreg or the reg itself > (noce_bbs_ok_for_cond_zero_arith): check if BBs are OK for condtional zero based ifcvt > (noce_process_if_block): add noce_try_cond_zero_arith > > gcc/testsuite/ChangeLog: > > * gcc.target/riscv/zicond_ifcvt_opt.c: New test. > --- > gcc/ifcvt.cc | 210 ++++++ > .../gcc.target/riscv/zicond_ifcvt_opt.c | 682 ++++++++++++++++++ > 2 files changed, 892 insertions(+) > create mode 100644 gcc/testsuite/gcc.target/riscv/zicond_ifcvt_opt.c > > diff --git a/gcc/ifcvt.cc b/gcc/ifcvt.cc > index a0af553b9ff..8f6a0e7f5fe 100644 > --- a/gcc/ifcvt.cc > +++ b/gcc/ifcvt.cc > @@ -787,6 +787,7 @@ static rtx noce_get_alt_condition (struct noce_if_info *, rtx, rtx_insn **); > static bool noce_try_minmax (struct noce_if_info *); > static bool noce_try_abs (struct noce_if_info *); > static bool noce_try_sign_mask (struct noce_if_info *); > +static int noce_try_cond_zero_arith (struct noce_if_info *); > > /* Return the comparison code for reversed condition for IF_INFO, > or UNKNOWN if reversing the condition is not possible. */ > @@ -1831,6 +1832,40 @@ noce_emit_cmove (struct noce_if_info *if_info, rtx x, enum rtx_code code, > return NULL_RTX; > } > > +/* Emit a conditional zero, returning TARGET or NULL_RTX upon failure. > + IF_INFO describes the if-conversion scenario under consideration. > + CZERO_CODE selects the condition (EQ/NE). > + NON_ZERO_OP is the nonzero operand of the conditional move > + TARGET is the desired output register. */ > + > +static rtx > +noce_emit_czero (struct noce_if_info *if_info, enum rtx_code czero_code, > + rtx non_zero_op, rtx target) [ ... ] The code you wrote is safe in that if constructs a suitable if-then-else as a single object, starts a new sequence the uses emit_insn to put that object onto a sequence. Then you extract that one and only one insn from the sequence and validate it can be recognized. In cases where you want to do something like this and know you're going to emit one and only one insn you can use 'make_insn_raw' without generating a new sequence. I would suggest you replace all the code starting with start_sequence() and ending with end_sequence () (inclusive) with something like insn = make_insn_raw (set); if (recog_memoized (insn) >= 0) { emit_insn (insn); return target; } return NULL_RTX; Note that in the future (gcc-15) when this code is generalized to use the expander it will potentially generate multiple insns at which point we'll have to put them on a sequence and validate they all are recognizable. But we'll tackle that at the appropriate time. > + > + return false; > +} > + > +/* Helper function to return REG itself or inner expression of a SUBREG, > + otherwise NULL_RTX for other RTX_CODE. */ > + > +static rtx > +get_base_reg (rtx exp) > +{ > + if (REG_P (exp)) > + return exp; > + else if (SUBREG_P (exp)) > + return SUBREG_REG (exp); > + > + return NULL_RTX; > + I would advise against handling subregs at this point. What you're doing is probably too simplistic given the semantics of subregs. > + > + /* Strip sign_extend if any. */ > + if (GET_CODE (a) == SIGN_EXTEND || GET_CODE (a) == ZERO_EXTEND) > + bin_exp = XEXP (a, 0); > + else > + bin_exp = a; Similarly while I do think we're going to want to handle extensions, let's not try and add them at this point. We want to get this wrapped up & integrated so that everyone can move their focus to bugfixing for gcc-14. > + > +/* Try to covert if-then-else with conditional zero, > + returning TURE on success or FALSE on failure. > + IF_INFO describes the if-conversion scenario under consideration. */ > + > +static int > +noce_try_cond_zero_arith (struct noce_if_info *if_info) > +{ > + rtx target, a; > + rtx_insn *seq; > + machine_mode mode = GET_MODE (if_info->x); > + rtx common = NULL_RTX; > + enum rtx_code czero_code = UNKNOWN; > + rtx non_zero_op = NULL_RTX; > + rtx *to_replace = NULL; > + > + if (!noce_bbs_ok_for_cond_zero_arith (if_info, &common, &czero_code, &a, > + &to_replace)) > + return false; > + > + /* Disallow CONST_INT currently for simplicity*/ > + if (to_replace == NULL || !REG_P (*to_replace)) > + return false;If you're not going to allow CONST_INT reject them in the ok_for_condzero_arith routine. Presumably you're rejecting constants because zicond doesn't allow them in the arms. We'll want to handle them in the future and can do so easily once we're using the expander. > + > + non_zero_op = *to_replace; > + > + start_sequence (); > + > + /* If x is used in both input and out like x = c ? x + z : x, > + use a new reg to avoid modifying x */ > + if (common && rtx_equal_p (common, if_info->x)) > + target = gen_reg_rtx (mode); > + else > + target = if_info->x; > + > + target = noce_emit_czero (if_info, czero_code, non_zero_op, target); > + if (!target || !to_replace) > + { > + end_sequence (); > + return false; > + } > + > + *to_replace = target; > + emit_insn (gen_rtx_SET (if_info->x, a)); This isn't necessarily safe. Use emit_move_insn instead. This is pretty close. Hopefully one more iteration and it can be integrated. jeff