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 C71113858C66 for ; Sun, 29 Oct 2023 10:49:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C71113858C66 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 C71113858C66 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=1698576577; cv=none; b=IdiZc8YOkrLTKcK3+UqpTk8u/YpAqUYeFDc0fstyaRNHD9cyeHDtgLQo57qiKSswF4Zt4/3BWG6l6ZaovUbH1Dgjp8o50gH4cz1EGcechCC95ZuLpgqAUCx3y9jRt+n/EV0grflFQ8pebPlqRPwQyRWo0WhX8T6v4HRRbJM3jv4= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698576577; c=relaxed/simple; bh=oJ79qZ+PfdLW1OHJW5FNA+FBSwc7tofDLDIKAwySm1Q=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:From:To; b=wTYNec7VnxBAvmeMWGYyU5Jo/Ep09+rNTcJyTTKYFychoKCYnbNovF+pXrhamsiDkq4Syu3aRt/atYSQ95vfJhHYSHkGzm3ufe33BcrIdDD4PcbgzBpukZ/5IISJM9ygyZGlQLXprwYVaP4ljxsZSN8MILZXyUAzzSAexjnYCAk= 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 39T9eaqd000937; Sun, 29 Oct 2023 10:49:34 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ibm.com; h=message-id : date : mime-version : subject : from : to : cc : references : in-reply-to : content-type : content-transfer-encoding; s=pp1; bh=/C8kZe3J34+dM/RO3SDU6CcHu1ssKp/M3h1PYa5rSmY=; b=E33d2PAEXWKCLiAwFO1uEEBTB2TJlNOH5WiistCjAmVtfU2B8yYkE6nxK0nvHp3TS/QV Y3Xv+ca+69sMBHPACsbBW0s8lgHxXsLE58PFML62m5+Y9EYWXKyWTxPrucnrMZ76xNqo XBRUTXQ5h3mIv1+A5Rlyg2O6ZMDN5Twcmnz3r3LCh7Te1QU8Em4BMlW0zQ/lNIEf+EXL 8pqyol96YuxP8EgugPdPFovF2/Q7sRPYCpAWXpt4t/V73LQ+uaqVunqnUh1IEye555Ym ta0XLjacrmbJu22KqedVt3LtX/sudJS7f3feL7O7ucTl0H59Cz1WL8ruaUeNzczYnM6Y fQ== Received: from pps.reinject (localhost [127.0.0.1]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3u1ax51rty-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 29 Oct 2023 10:49:34 +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 39TAe6rX001232; Sun, 29 Oct 2023 10:49:34 GMT Received: from ppma22.wdc07v.mail.ibm.com (5c.69.3da9.ip4.static.sl-reverse.com [169.61.105.92]) by mx0a-001b2d01.pphosted.com (PPS) with ESMTPS id 3u1ax51rtt-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 29 Oct 2023 10:49:33 +0000 Received: from pps.filterd (ppma22.wdc07v.mail.ibm.com [127.0.0.1]) by ppma22.wdc07v.mail.ibm.com (8.17.1.19/8.17.1.19) with ESMTP id 39TAn8iA019876; Sun, 29 Oct 2023 10:49:32 GMT Received: from smtprelay03.wdc07v.mail.ibm.com ([172.16.1.70]) by ppma22.wdc07v.mail.ibm.com (PPS) with ESMTPS id 3u1d0y2mgu-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Sun, 29 Oct 2023 10:49:32 +0000 Received: from smtpav05.dal12v.mail.ibm.com (smtpav05.dal12v.mail.ibm.com [10.241.53.104]) by smtprelay03.wdc07v.mail.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id 39TAnWgq22938120 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Sun, 29 Oct 2023 10:49:32 GMT Received: from smtpav05.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id E3A0258065; Sun, 29 Oct 2023 10:49:31 +0000 (GMT) Received: from smtpav05.dal12v.mail.ibm.com (unknown [127.0.0.1]) by IMSVA (Postfix) with ESMTP id B6BCC58052; Sun, 29 Oct 2023 10:49:28 +0000 (GMT) Received: from [9.43.5.174] (unknown [9.43.5.174]) by smtpav05.dal12v.mail.ibm.com (Postfix) with ESMTP; Sun, 29 Oct 2023 10:49:28 +0000 (GMT) Message-ID: Date: Sun, 29 Oct 2023 16:19:28 +0530 MIME-Version: 1.0 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 From: Ajit Agarwal To: Vineet Gupta , Bernhard Reutner-Fischer Cc: gcc-patches@gcc.gnu.org, 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> <20231023161027.362c626b@nbbrfq.loc> <8da41716-1111-4550-95dd-de41a402101e@linux.ibm.com> <4077DE16-87DA-4DDE-B119-6B516944B632@gmail.com> <7379c85b-178c-4196-a929-129052245165@linux.ibm.com> <20231027191612.068c67ee@nbbrfq.loc> <1f218675-3196-4b51-b08a-bb122501c306@rivosinc.com> <7abe07ee-2a27-4f07-9545-b5d4f977c611@linux.ibm.com> In-Reply-To: <7abe07ee-2a27-4f07-9545-b5d4f977c611@linux.ibm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-TM-AS-GCONF: 00 X-Proofpoint-ORIG-GUID: uLa7a_EAQ-qEK5OcxG-29ZaVo9ud9e-W X-Proofpoint-GUID: H3UVrquv1ZB3cVpPqavf-RWj5RPYHpvV X-Proofpoint-Virus-Version: vendor=baseguard engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26 definitions=2023-10-28_24,2023-10-27_01,2023-05-22_02 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 phishscore=0 suspectscore=0 impostorscore=0 spamscore=0 mlxscore=0 adultscore=0 mlxlogscore=999 priorityscore=1501 clxscore=1015 bulkscore=0 lowpriorityscore=0 malwarescore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000 definitions=main-2310290094 X-Spam-Status: No, score=-4.1 required=5.0 tests=BAYES_00,BODY_8BITS,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_EF,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 28/10/23 3:56 pm, Ajit Agarwal wrote: > > > On 28/10/23 4:09 am, Vineet Gupta wrote: >> >> >> On 10/27/23 10:16, Bernhard Reutner-Fischer wrote: >>> On Wed, 25 Oct 2023 16:41:07 +0530 >>> Ajit Agarwal wrote: >>> >>>> On 25/10/23 2:19 am, Vineet Gupta wrote: >>>>> On 10/24/23 13:36, rep.dot.nop@gmail.com wrote: >>>>>>>>>> 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? >>>>>>>> The function below looks identical in v12 of the patch. >>>>>>>> Why didn't you use common subexpressions? >>>>>>>> ba >>>>>>> Using CSE here breaks aarch64 regressions hence I have reverted it back >>>>>>> not to use CSE, >>>>>> Just for my own education, can you please paste your patch perusing common subexpressions and an assembly diff of the failing versus working aarch64 testcase, along how you configured that failing (cross-?)compiler and the command-line of a typical testcase that broke when manually CSEing the function below? >>>>> I was meaning to ask this before, but what exactly is the CSE issue, manually or whatever. >>> If nothing else it would hopefully improve the readability. >>> >>>>>    >>>> Here is the abi interface where I CSE'D and got a mail from automated regressions run that aarch64 >>>> test fails. >>> We already concluded that this failure was obviously a hiccup on the >>> testers, no problem. >>> >>>> +static inline bool >>>> +abi_extension_candidate_return_reg_p (int regno) >>>> +{ >>>> +  return targetm.calls.function_value_regno_p (regno); >>>> +} >>> But i was referring to abi_extension_candidate_p :) >>> >>> your v13 looks like this: >>> >>> +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 (REGNO (orig_src))) >>> +    return false; >>> + >>> +  /* Return FALSE if mode of destination and source is same.  */ >>> +  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); >>> + >>> +  /* Return FALSE if promote is false and REGNO of source and destination >>> +     is different.  */ >>> +  if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src)) >>> +    return false; >>> + >>> +  return true; >>> +} >>> >>> and i suppose it would be easier to read if phrased something like >>> >>> static bool >>> abi_extension_candidate_p (rtx_insn *insn) >>> { >>>    rtx set = single_set (insn); >>>    rtx orig_src = XEXP (SET_SRC (set), 0); >>>    unsigned int src_regno = REGNO (orig_src); >>> >>>    /* Not a function argument reg or is a function values return reg.  */ >>>    if (!FUNCTION_ARG_REGNO_P (src_regno) >>>        || abi_extension_candidate_return_reg_p (src_regno)) >>>      return false; >>> >>>    rtx dst = SET_DST (set); >>>    machine_mode src_mode = GET_MODE (orig_src); >>> >>>    /* Return FALSE if mode of destination and source is the same.  */ >>>    if (GET_MODE (dst) == src_mode) >>>      return false; >>> >>>    /* Return FALSE if the FIX THE COMMENT and REGNO of source and destination >>>       is different.  */ >>>    if (!abi_target_promote_function_mode_p (src_mode) >>>        && REGNO (dst) != src_regno) >>>      return false; >>> >>>    return true; >>> } >>> >>> so no, that's not exactly better. >>> >>> Maybe just do what the function comment says (i did not check the "not >>> promoted" part, but you get the idea): >>> >>> ^L >>> >>> /* Return TRUE if >>>     reg source operand is argument register and not return register, >>>     mode of source and destination operand are different, >>>     if not promoted REGNO of source and destination operand are the same.  */ >>> static bool >>> abi_extension_candidate_p (rtx_insn *insn) >>> { >>>    rtx set = single_set (insn); >>>    rtx orig_src = XEXP (SET_SRC (set), 0); >>> >>>    if (FUNCTION_ARG_REGNO_P (REGNO (orig_src)) >>>        && !abi_extension_candidate_return_reg_p (REGNO (orig_src)) >>>        && GET_MODE (SET_DST (set)) != GET_MODE (orig_src) >>>        && abi_target_promote_function_mode_p (GET_MODE (orig_src)) >>>        && REGNO (SET_DST (set)) == REGNO (orig_src)) >>>      return true; >>> >>>    return false; >>> } >> >> This may have been my doing as I asked to split out the logic as some of the conditions merit more commentary. >> e.g. why does the mode need to be same >> But granted this is the usual coding style in gcc and the extra comments could still be added before the big if >> > > Addressed in V15 of the patch, The above rearranging code breaks the logic. I have implemented as follows. +/* Return TRUE if + reg source operand is argument register and not return register, + mode of source and destination operand are different, + if not promoted REGNO of source and destination operand are the 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 (REGNO (orig_src)) + && dst_mode != GET_MODE (orig_src)) + { + if (!abi_target_promote_function_mode (GET_MODE (orig_src)) + && REGNO (SET_DEST (set)) != REGNO (orig_src)) + return false; + + return true; + } + return false; +} Thanks & Regards Ajit >> -Vineet >> >>> >>> I think this is much easier to actually read (and that's why good >>> function comments are important). In the end it's not important and >>> just personal preference. >>> Either way, I did not check the plausibility of the logic therein. >>> >>>> >>>> I have not done any assembly diff as myself have not cross compiled with aarch64. >>> fair enough. >>