From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-001b2d01.pphosted.com (mx0a-001b2d01.pphosted.com [148.163.156.1]) by sourceware.org (Postfix) with ESMTPS id 461503858D37 for ; Tue, 24 Oct 2023 07:40:55 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 461503858D37 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 461503858D37 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=148.163.156.1 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698133257; cv=none; b=JLLDpAPzvjUxRiO1ts3CIcDFa/vFMkHGnj81i3yuHtwvMJGeMCMhH8FAw3UCfl0o2MH4wspQ8tkBQLUuenYfIXM4ZW02p84zjnCRkATch15xq9KxvsWovuNOuGs86jm8xaV0xRw811zr0Hb2QRb1drd+BSlN5iUF6566SsDxu1k= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698133257; c=relaxed/simple; bh=q0JY6fUsRsnSKIImZQiUf4+38qz7RCYsL5sRHBYiXpo=; h=DKIM-Signature:Message-ID:Date:Subject:To:From:MIME-Version; b=G3HxRnk414HVdLwGJ9R6ADa1MToKMEf7XrZx3inF1fw94O3TzsXC6r8ikgH3gpMy68YfUQqA3ZtrPeqW3vSGF310VRukoqfW6RBF/2Jz1wM7ujB2PwDUWWRCUTnNcYGQx5MLyUSdL3wvnm9/UR8jfN8K17a2GIKAniyE7rNzjso= ARC-Authentication-Results: i=1; server2.sourceware.org Received: from pps.filterd (m0353729.ppops.net [127.0.0.1]) by mx0a-001b2d01.pphosted.com (8.17.1.19/8.17.1.19) with ESMTP id 39O7bLcr002744; Tue, 24 Oct 2023 07:40:53 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : subject : to : cc : references : from : in-reply-to : content-type : content-transfer-encoding : mime-version; s=pp1; bh=o/sTYFpieZa8+IxExNyR0vqfMd4kyjIu8lxAxD3zAm0=; b=hGxCslyOeEtuX6gY65ZNTeKF5StfNdQ0Zen80eUgm9N9uvzEeEAbpKNoeKV01kRlP4D6 ApR2G04TI2EAMNaJIKkVB+Do3txme5zGoZk7vOPNYnWVpy1vGadE0+R1X4orBf/W5UYz sfcBQlb0UQ3S1x+h6z3I6MmcyZnmxNXAh0o7vXmrgXaIpBm8AWhiyJNHHDD7lcBxIPUX ANQ8XijlMUfxKwxZWRPWTp2dl7kwXbaKr+BfX2v3Fp2Gaj3Al3GA3ON9VkdotzJ54Ozh 6qSbSg3dxU1oYR6QQy/+1SZmBh+9yFuWEg9QIb4kidgt9uY36DKgFStUED6DnnZijA2T ow== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3tx9ptr44w-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Oct 2023 07:40:52 +0000 Received: from m0353729.ppops.net (m0353729.ppops.net [127.0.0.1]) by pps.reinject (8.17.1.5/8.17.1.5) with ESMTP id 39O7eEXc014426; Tue, 24 Oct 2023 07:40:52 GMT Received: from ppma23.wdc07v.mail.ibm.com (5d.69.3da9.ip4.static.sl-reverse.com [169.61.105.93]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3tx9ptr42u-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Oct 2023 07:40:52 +0000 Received: from pps.filterd (ppma23.wdc07v.mail.ibm.com [127.0.0.1]) by ppma23.wdc07v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 39O5wlDS005004; Tue, 24 Oct 2023 07:40:48 GMT Received: from smtprelay02.wdc07v.mail.ibm.com ([172.16.1.69]) by ppma23.wdc07v.mail.ibm.com (PPS) with ESMTPS id 3tvtfkdr40-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Tue, 24 Oct 2023 07:40:48 +0000 Received: from smtpav01.wdc07v.mail.ibm.com (smtpav01.wdc07v.mail.ibm.com [10.39.53.228]) by smtprelay02.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 39O7emuv6029896 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 24 Oct 2023 07:40:48 GMT Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 726285804B; Tue, 24 Oct 2023 07:40:48 +0000 (GMT) Received: from smtpav01.wdc07v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id 2451458066; Tue, 24 Oct 2023 07:40:45 +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 07:40:44 +0000 (GMT) Message-ID: Date: Tue, 24 Oct 2023 13:10:43 +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: Vineet Gupta , rep.dot.nop@gmail.com, gcc-patches@gcc.gnu.org Cc: Jeff Law , Richard Biener , Segher Boessenkool , Peter Bergner , gnu-toolchain References: <32ca6e0e-ef68-4d4d-b864-c586a688b2c7@linux.ibm.com> <22541c92-a967-4e66-96b3-e4ad5011cd24@rivosinc.com> <6c2c1b77-b341-4abc-b572-e402d0d14d8b@rivosinc.com> From: Ajit Agarwal In-Reply-To: <6c2c1b77-b341-4abc-b572-e402d0d14d8b@rivosinc.com> Content-Type: text/plain; charset=UTF-8 X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: XZAlWQmbyGzHm5_3EMmc1KuTacFZYM-N X-Proofpoint-GUID: f4CYZ2N-mjfsNgyiLCMvgpn4AMPoMtxm 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_06,2023-10-19_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 clxscore=1015 mlxscore=0 malwarescore=0 suspectscore=0 priorityscore=1501 phishscore=0 bulkscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 lowpriorityscore=0 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310170001 definitions=main-2310240063 X-Spam-Status: No, score=-3.6 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: 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. > 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 >