From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x229.google.com (mail-lj1-x229.google.com [IPv6:2a00:1450:4864:20::229]) by sourceware.org (Postfix) with ESMTPS id 660B73858D35 for ; Sat, 21 Oct 2023 19:27:07 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 660B73858D35 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 660B73858D35 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::229 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697916430; cv=none; b=RMnefUrH2Fai+lZISE171TsE77fI1pKEOY5q4wz4V4cacP9nJkHdT541oHBxuyqRe9gmOTm2HeCdtdo3Eu2YMc8CdEYBsKbCHiFN0bXca/4h/dpbpVWTxak4haoVqA3qXJTj0GT7Np7ITPstQk4tRNTGMSzaVB0a8u92QXVv3wc= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1697916430; c=relaxed/simple; bh=bdKBWp/msjVgNCnJEjWu74w9zTyGnIggQi36y7md3u0=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=D3wxjexQ3AYuCIvtNqltGIzXcWWq7hDHHFhs1uCiQnd1/j9I6fwH3HBM7K6fS/m5snPBOqfGodH9DWxtIpr/XaNl9GgRSRltDahudPGEHrWhPT5NBJyURgVl5CNyNN7v1uGyDQ5qwmT41ProLCDaI0nvoHhT+pBrQDI6A/hRtLU= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-x229.google.com with SMTP id 38308e7fff4ca-2c518a1d83fso31555341fa.3 for ; Sat, 21 Oct 2023 12:27:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697916425; x=1698521225; darn=gcc.gnu.org; h=content-transfer-encoding:mime-version:message-id:references :in-reply-to:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=1YtCnOpPNo9TmepItRVE0wFnyupageK03FK6ErjGGrE=; b=WREzRmdpjKeVJcIXO3WJ4wpUrL7SU86amQgL27SKe2o4CibnWwWcdzjhnqm4R2tWXB Z67UDu0h9v2pYl6FCIUPkMiyNlGpUrciHIArd+SmhbWe6WrnfFSlI1svW+sJPbCg3mfO 5TOcHyC0XayiXZwMIfrKtlgl1Nsak1E9z1kL5RSG1x9jSu+sz7IKW26exTJm/Gy8W4uc 71Wdh/2jYy37iGc9o9qlOLKLpmuZ3/nFt0Niypr1i7nFVg6w6rdkRP+G+2cItDUOIWGr OMwnIYzBITDcagVzl6Yu7/yuei9w00UgUOfKIpmiaTtdjrBKs2Im8rmHJ6umVO2t2okf aNGw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697916425; x=1698521225; h=content-transfer-encoding:mime-version:message-id:references :in-reply-to:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1YtCnOpPNo9TmepItRVE0wFnyupageK03FK6ErjGGrE=; b=c9kDZkBdihrMd6fFsJIs1YlwtGLt123VmYDkJmyaCQ/TpyjudiJI4LebAbON4PkXc9 f5A0BfE+sNuoaDy8W3PPe+bN4QueguqhDUBelRrMHfFwguHMRw5m61QrnB7or6EHSi/k k5FptbnmYV7OEJrsTc4yCt+HboEKzuQLF6z8B4y3Nshl4yhjRi3e3/YmjHBvlwhAn9b9 +A9Jly/BCasUdBv0/sEY+1QZstqcVHuAmjXJVXdBaAlBXx9RFzspyNxbxHqpKiMz6hnd pMJqL1YJNyBM3Gyyii/LeIlgxERUF0YXzhYWoFk4OVRLqrN/mITSibstKS6nqvKW0Pea eu+Q== X-Gm-Message-State: AOJu0YwzX5wnj9NHqO9hbJcrTGfpGaD7escN4kG/RA0/RdqNUvRKZFqO A36vImn9kqnUASVk9PMlUasmev1VGGQ= X-Google-Smtp-Source: AGHT+IE/vX+7/h3E8v6vOutoV3o88NTrARSrqUe2XYu7b11VXcJgITqZk4aXo6NTMWnwBvzPO2VCoA== X-Received: by 2002:a19:8c08:0:b0:500:8022:3dc7 with SMTP id o8-20020a198c08000000b0050080223dc7mr3200789lfd.10.1697916425191; Sat, 21 Oct 2023 12:27:05 -0700 (PDT) Received: from ?IPv6:::1? ([2001:871:227:8210:3dca:d1d1:23b5:8bc7]) by smtp.gmail.com with ESMTPSA id n25-20020a509359000000b00530bc7cf377sm3863405eda.12.2023.10.21.12.27.04 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 21 Oct 2023 12:27:04 -0700 (PDT) Date: Sat, 21 Oct 2023 21:26:58 +0200 From: rep.dot.nop@gmail.com To: gcc-patches@gcc.gnu.org, Vineet Gupta , Ajit Agarwal , gcc-patches CC: Jeff Law , Richard Biener , Segher Boessenkool , Peter Bergner , gnu-toolchain Subject: =?US-ASCII?Q?Re=3A_=5BPATCH_v9_4/4=5D_ree=3A_Improve_ree_pass_fo?= =?US-ASCII?Q?r_rs6000_target_using_defined_ABI_interfaces?= In-Reply-To: <22541c92-a967-4e66-96b3-e4ad5011cd24@rivosinc.com> References: <32ca6e0e-ef68-4d4d-b864-c586a688b2c7@linux.ibm.com> <22541c92-a967-4e66-96b3-e4ad5011cd24@rivosinc.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP 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 21 October 2023 01:56:16 CEST, Vineet Gupta wro= te: >On 10/19/23 23:50, Ajit Agarwal wrote: >> Hello All: >>=20 >> This version 9 of the patch uses abi interfaces to remove zero and sign= extension elimination=2E >> Bootstrapped and regtested on powerpc-linux-gnu=2E >>=20 >> In this version (version 9) of the patch following review comments are = incorporated=2E >>=20 >> a) Removal of hard code zero_extend and sign_extend in abi interfaces= =2E >> b) Source and destination with different registers are considered=2E >> c) Further enhancements=2E >> d) Added sign extension elimination using abi interfaces=2E > >As has been trend in the past, I don't think all the review comments have= been addressed=2E And apart from that, may I ask if this is just me, or does anybody else th= ink that it might be worthwhile to actually read a patch before (re-)postin= g? Seeing e=2Eg=2E the proposed abi_extension_candidate_p as written in a fir= st POC would deserve some manual CSE, if nothing more then for clarity and = conciseness? Just curious from a meta perspective=2E=2E And: >> ree: Improve ree pass for rs6000 target using defined abi interfaces mentioning powerpc like this, and then changing generic code could be inte= rpreted as misleading, IMHO=2E >>=20 >> For rs6000 target we see redundant zero and sign extension and done >> to improve ree pass to eliminate such redundant zero and sign extension >> using defined ABI interfaces=2E Mentioning powerpc in the body as one of the affected target(s) is of cour= se fine=2E >> +/* Return TRUE if target mode is equal to source mode of zero_extend >> + or sign_extend otherwise false=2E */ , false otherwise=2E But I'm not a native speaker=20 >> +/* Return TRUE if the candidate insn is zero extend and regno is >> + a return registers=2E */ >> + >> +static bool >> +abi_extension_candidate_return_reg_p (/*rtx_insn *insn, */int regno) Leftover debug comment=2E >> +{ >> + if (targetm=2Ecalls=2Efunction_value_regno_p (regno)) >> + return true; >> + >> + return false; >> +} >> + As said, I don't see why the below was not cleaned up before the V1 submis= sion=2E Iff it breaks when manually CSEing, I'm curious why? >> +/* Return TRUE if reg source operand of zero_extend is argument regist= ers >> + and not return registers and source and destination operand are sam= e >> + and mode of source and destination operand are not same=2E */ >> + >> +static bool >> +abi_extension_candidate_p (rtx_insn *insn) >> +{ >> + rtx set =3D single_set (insn); >> + machine_mode dst_mode =3D GET_MODE (SET_DEST (set)); >> + rtx orig_src =3D XEXP (SET_SRC (set), 0); >> + >> + if (!FUNCTION_ARG_REGNO_P (REGNO (orig_src)) >> + || abi_extension_candidate_return_reg_p (/*insn,*/ REGNO (orig_s= rc))) On top, debug leftover=2E >> + return false; >> + >> + /* Mode of destination and source should be different=2E */ >> + if (dst_mode =3D=3D GET_MODE (orig_src)) >> + return false; >> + >> + machine_mode mode =3D GET_MODE (XEXP (SET_SRC (set), 0)); >> + bool promote_p =3D abi_target_promote_function_mode (mode); >> + >> + /* REGNO of source and destination should be same if not >> + promoted=2E */ >> + if (!promote_p && REGNO (SET_DEST (set)) !=3D REGNO (orig_src)) >> + return false; >> + >> + return true; >> +} >> + As said, please also rephrase the above (and everything else if it obvious= ly looks akin the above)=2E The rest, mentioned below, should largely be covered by following the cod= ing convention=2E >> +/* Return TRUE if the candidate insn is zero extend and regno is >> + an argument registers=2E */ Singular register=2E >> + >> +static bool >> +abi_extension_candidate_argno_p (/*rtx_code code, */int regno) Debug leftover=2E I would probably have inlined this function manually, with a respective co= mment=2E Did not look how often it is used, admittedly=2E >> +{ >> + if (FUNCTION_ARG_REGNO_P (regno)) >> + return true; >> + >> + return false; >> +} [] >> + >> /* This function goes through all reaching defs of the source s/This function goes/Go/ >> of the candidate for elimination (CAND) and tries to combine (of, ?didn't look) candidate CAND for eliminating >> the extension with the definition instruction=2E The changes defining Pre-existing, I know=2E But you could fix those in a preparatory patch while you touch surrounding= code=2E This is not a requirement, of course, just good habit, IMHO=2E >> @@ -770,6 +889,11 @@ combine_reaching_defs (ext_cand *cand, const_rtx s= et_pat, ext_state *state) >> state->defs_list=2Etruncate (0); >> state->copies_list=2Etruncate (0); >> + rtx orig_src =3D XEXP (SET_SRC (cand->expr),0); >> + >> + if (abi_extension_candidate_p (cand->insn) >> + && (!get_defs (cand->insn, orig_src, NULL))) Excess braces=2E Hopefully check_gnu_style would have complained=2E >> + return abi_handle_regs (cand->insn); >> outcome =3D make_defs_and_copies_lists (cand->insn, set_pat, stat= e); >> @@ -1036,6 +1160,15 @@ combine_reaching_defs (ext_cand *cand, const_r= tx set_pat, ext_state *state) >> } >> } >> + rtx insn_set =3D single_set (cand->insn); >> + >> + machine_mode mode =3D (GET_MODE (XEXP (SET_SRC (insn_set), 0))); Excess braces=2E Also in a lot of other spots in your patch=2E Please read the coding conventions and the patch, once again, before submi= ssion? thanks