From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0b-001b2d01.pphosted.com (mx0b-001b2d01.pphosted.com [148.163.158.5]) by sourceware.org (Postfix) with ESMTPS id 208183858D37 for ; Tue, 24 Oct 2023 09:36:57 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 208183858D37 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=linux.ibm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linux.ibm.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 208183858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.158.5 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698140219; cv=none; b=U1NCFzk7SxVDaCDt40dSHA7dgzfuKTx4rxq5c8BxxCs50lzt2Fq05E8AyhILfhPw1WSeq/6eCtVz7+Vbpuf0B/Lshjdg7nOQdnwFnF2K0yeEctB0TaNLhU70q8vh0YG1dSXIOyyRlKGVAGN5zdHcwr1SWvoT1Ysrcakvg/bHkz0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698140219; c=relaxed/simple; bh=RAUG7pDIMw/yIRH/Y4Rg8XStoacTRk0n0n0maDtEaqk=; h=DKIM-Signature:Message-ID:Date:Subject:To:From:MIME-Version; b=TWO8ODISZY0AMsjXg6yigTTLSf88L9jHAqq477NW20j8sF+mhczpB6DqzpLPtJ7DOnuMMfToKK3n1+GCnH7wGGBAbIXveTOq547ckw61hm77idruLdgOrQkxGHkaBXF6XsrPg4zlTTuFpxNIY/UWPtA/9egQIL2euwdHjLzoH68= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0356516.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 39O9Z6vD009751; Tue, 24 Oct 2023 09:36:56 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : subject : to : references : cc : from : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pp1; bh=3+FAeew3R6HbqpX0WLhgiy1rj5pVRQnvzBmrPN9Onrc=; b=kPlfQm5BPpD1vgjDrHCfEOF6ha4KxdDakZaV8BZZNEtOpuyxg9V9rx2/jcSVoWQsGdK7 LARsXOZqCTKFg6iX5WV6qhSuUW9odkD278EYL8X5/RmnubQEhv44+HRli+MwmxPsYBux dx2LUarzG4QPBrx318yz1wW3nu+b/GMxrdjijVxVqPkcHMcn5+hR3y3oSczB9DSxNJ4+ +PPfJ7UoKZ8m2H3Fv1913n+hvYQuDszJQqFOkPz+Hyv4sjoNuRdYNzlakevMahpubBp9 PsAPDof4Zvs+cuougRdq52Gm++wn2XfHIwIIAvAcqUCQ0YoXxoxGmZiZglCBCRunZsmL 7Q== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3txbdug2v4-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Oct 2023 09:36:56 +0000 Received: from m0356516.ppops.net (m0356516.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 39O9ZJFI010317; Tue, 24 Oct 2023 09:36:55 GMT Received: from ppma12.dal12v.mail.ibm.com (dc.9e.1632.ip4.static.sl-reverse.com [50.22.158.220]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3txbdug2up-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Oct 2023 09:36:55 +0000 Received: from pps.filterd (ppma12.dal12v.mail.ibm.com [127.0.0.1]) by ppma12.dal12v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 39O7K7Hv023789; Tue, 24 Oct 2023 09:36:55 GMT Received: from smtprelay07.dal12v.mail.ibm.com ([172.16.1.9]) by ppma12.dal12v.mail.ibm.com (PPS) with ESMTPS id 3tvrysxtbw-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Oct 2023 09:36:55 +0000 Received: from smtpav01.wdc07v.mail.ibm.com (smtpav01.wdc07v.mail.ibm.com [10.39.53.228]) by smtprelay07.dal12v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 39O9as7n51905264 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Oct 2023 09:36:54 GMT Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 5A3255804B; Tue, 24 Oct 2023 09:36:54 +0000 (GMT) Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 3E13E58059; Tue, 24 Oct 2023 09:36:51 +0000 (GMT) Received: from [9.43.52.246] (unknown [9.43.52.246]) by smtpav01.wdc07v.mail.ibm.com (Postfix) with ESMTP; Tue, 24 Oct 2023 09:36:50 +0000 (GMT) Message-ID: <590b4da3-ac9d-48b5-9f48-9f270b420c88@linux.ibm.com> Date: Tue, 24 Oct 2023 15:06:49 +0530 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v9 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces Content-Language: en-US To: gcc-patches@gcc.gnu.org, Vineet Gupta , Jeff Law References: <32ca6e0e-ef68-4d4d-b864-c586a688b2c7@linux.ibm.com> <22541c92-a967-4e66-96b3-e4ad5011cd24@rivosinc.com> <6c2c1b77-b341-4abc-b572-e402d0d14d8b@rivosinc.com> Cc: Segher Boessenkool , Richard Biener , Peter Bergner , Bernhard Reutner-Fischer From: Ajit Agarwal In-Reply-To: Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: yXxWwVqG3NEMjGLflmoloTd76QJjXrl0 X-Proofpoint-GUID: ThmJlvR2Y98BRVf-B3Sll3d_AN1FhpFG Content-Transfer-Encoding: 8bit X-Proofpoint-UnRewURL: 0 URL was un-rewritten MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.980,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-24_07,2023-10-19_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 mlxscore=0 mlxlogscore=999 clxscore=1015 priorityscore=1501 phishscore=0 impostorscore=0 spamscore=0 malwarescore=0 lowpriorityscore=0 bulkscore=0 suspectscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001 definitions=main-2310240081 X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,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 24/10/23 1:10 pm, Ajit Agarwal wrote: > Hello Vineet: > > On 24/10/23 12:02 am, Vineet Gupta wrote: >> >> >> On 10/22/23 23:46, Ajit Agarwal wrote: >>> Hello All: >>> >>> Addressed below review comments in the version 11 of the patch. >>> Please review and please let me know if its ok for trunk. >>> >>> Thanks & Regards >>> Ajit >> >> Again you are not paying attention to prior comments about fixing your submission practice and like some of the prior reviewers I'm starting to get tired, despite potentially good technical content. >> > > Sorry for the inconvenience caused. I will make sure all the comments from reviewers > are addressed. > >> 1. The commentary above is NOT part of changelog. Either use a separate cover letter or add patch version change history between two "---" lines just before the start of code diff. And keep accumulating those as you post new versions. See [1]. This is so reviewers knwo what changed over 10 months and automatically gets dropped when patch is eventually applied/merged into tree. >> > > Sure I will do that. Made changes in version 13 of the patch with changes since v6. Thanks & Regards Ajit > >> 2. Acknowledge (even if it is yes) each and every comment of the reviewerw explicitly inline below. That ensures you don't miss addressing a change since this forces one to think about each of them. >> > > Surely I will acknowledge each and every comments inline. > >> I do have some technical comments which I'll follow up with later. > > I look forward to it. > >> Just a short summary that v10 indeed bootstraps risc-v but I don't see any improvements at all - as in whenever abi interfaces code identifies and extension (saw missing a definition, the it is not able to eliminate any extensions despite the patch. >> > > Thanks for the summary and the check. > > Thanks & Regards > Ajit > >> -Vineet >> >> [1] https://gcc.gnu.org/pipermail/gcc-patches/2023-October/632180.html >> >>> >>> On 22/10/23 12:56 am, rep.dot.nop@gmail.com wrote: >>>> On 21 October 2023 01:56:16 CEST, Vineet Gupta wrote: >>>>> On 10/19/23 23:50, Ajit Agarwal wrote: >>>>>> Hello All: >>>>>> >>>>>> This version 9 of the patch uses abi interfaces to remove zero and sign extension elimination. >>>>>> Bootstrapped and regtested on powerpc-linux-gnu. >>>>>> >>>>>> In this version (version 9) of the patch following review comments are incorporated. >>>>>> >>>>>> a) Removal of hard code zero_extend and sign_extend  in abi interfaces. >>>>>> b) Source and destination with different registers are considered. >>>>>> c) Further enhancements. >>>>>> d) Added sign extension elimination using abi interfaces. >>>>> As has been trend in the past, I don't think all the review comments have been addressed. >>>> And apart from that, may I ask if this is just me, or does anybody else think that it might be worthwhile to actually read a patch before (re-)posting? >>>> >>>> Seeing e.g. the proposed abi_extension_candidate_p as written in a first POC would deserve some manual CSE, if nothing more then for clarity and conciseness? >>>> >>>> Just curious from a meta perspective.. >>>> >>>> And: >>>> >>>>>> ree: Improve ree pass for rs6000 target using defined abi interfaces >>>> mentioning powerpc like this, and then changing generic code could be interpreted as misleading, IMHO. >>>> >>>>>> 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. >>>> Mentioning powerpc in the body as one of the affected target(s) is of course fine. >>>> >>>> >>>>>>    +/* Return TRUE if target mode is equal to source mode of zero_extend >>>>>> +   or sign_extend otherwise false.  */ >>>> , false otherwise. >>>> >>>> But I'm not a native speaker >>>> >>>> >>>>>> +/* Return TRUE if the candidate insn is zero extend and regno is >>>>>> +   a return registers.  */ >>>>>> + >>>>>> +static bool >>>>>> +abi_extension_candidate_return_reg_p (/*rtx_insn *insn, */int regno) >>>> Leftover debug comment. >>>> >>>>>> +{ >>>>>> +  if (targetm.calls.function_value_regno_p (regno)) >>>>>> +    return true; >>>>>> + >>>>>> +  return false; >>>>>> +} >>>>>> + >>>> As said, I don't see why the below was not cleaned up before the V1 submission. >>>> Iff it breaks when manually CSEing, I'm curious why? >>>> >>>>>> +/* Return TRUE if reg source operand of zero_extend is argument registers >>>>>> +   and not return registers and source and destination operand are same >>>>>> +   and mode of source and destination operand are not same.  */ >>>>>> + >>>>>> +static bool >>>>>> +abi_extension_candidate_p (rtx_insn *insn) >>>>>> +{ >>>>>> +  rtx set = single_set (insn); >>>>>> +  machine_mode dst_mode = GET_MODE (SET_DEST (set)); >>>>>> +  rtx orig_src = XEXP (SET_SRC (set), 0); >>>>>> + >>>>>> +  if (!FUNCTION_ARG_REGNO_P (REGNO (orig_src)) >>>>>> +      || abi_extension_candidate_return_reg_p (/*insn,*/ REGNO (orig_src))) >>>> On top, debug leftover. >>>> >>>>>> +    return false; >>>>>> + >>>>>> +  /* Mode of destination and source should be different.  */ >>>>>> +  if (dst_mode == GET_MODE (orig_src)) >>>>>> +    return false; >>>>>> + >>>>>> +  machine_mode mode = GET_MODE (XEXP (SET_SRC (set), 0)); >>>>>> +  bool promote_p = abi_target_promote_function_mode (mode); >>>>>> + >>>>>> +  /* REGNO of source and destination should be same if not >>>>>> +      promoted.  */ >>>>>> +  if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src)) >>>>>> +    return false; >>>>>> + >>>>>> +  return true; >>>>>> +} >>>>>> + >>>> As said, please also rephrase the above (and everything else if it obviously looks akin the above). >>>> >>>> The rest, mentioned below,  should largely be covered by following the coding convention. >>>> >>>>>> +/* Return TRUE if the candidate insn is zero extend and regno is >>>>>> +   an argument registers.  */ >>>> Singular register. >>>> >>>>>> + >>>>>> +static bool >>>>>> +abi_extension_candidate_argno_p (/*rtx_code code, */int regno) >>>> Debug leftover. >>>> I would probably have inlined this function manually, with a respective comment. >>>> Did not look how often it is used, admittedly. >>>> >>>>>> +{ >>>>>> +  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.  The changes >>>> defining >>>> >>>> Pre-existing, I know. >>>> But you could fix those in a preparatory patch while you touch surrounding code. >>>> This is not a requirement, of course, just good habit, IMHO. >>>> >>>>>> @@ -770,6 +889,11 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) >>>>>>        state->defs_list.truncate (0); >>>>>>      state->copies_list.truncate (0); >>>>>> +  rtx orig_src = XEXP (SET_SRC (cand->expr),0); >>>>>> + >>>>>> +  if (abi_extension_candidate_p (cand->insn) >>>>>> +      && (!get_defs (cand->insn, orig_src, NULL))) >>>> Excess braces. >>>> Hopefully check_gnu_style would have complained. >>>> >>>>>> +    return abi_handle_regs (cand->insn); >>>>>>        outcome = make_defs_and_copies_lists (cand->insn, set_pat, state); >>>>>>    @@ -1036,6 +1160,15 @@ combine_reaching_defs (ext_cand *cand, const_rtx set_pat, ext_state *state) >>>>>>            } >>>>>>        } >>>>>>    +  rtx insn_set = single_set (cand->insn); >>>>>> + >>>>>> +  machine_mode mode = (GET_MODE (XEXP (SET_SRC (insn_set), 0))); >>>> Excess braces. >>>> Also in a lot of other spots in your patch. >>>> Please read the coding conventions and the patch, once again, before submission? >>>> thanks >>