From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x12f.google.com (mail-lf1-x12f.google.com [IPv6:2a00:1450:4864:20::12f]) by sourceware.org (Postfix) with ESMTPS id 77F243858C2B for ; Fri, 27 Oct 2023 17:16:21 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 77F243858C2B 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 77F243858C2B Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::12f ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698426983; cv=none; b=vU2CQ41PlKj16UmmUOxLr9SZ0ZqzAm/nlmIJg8cGosXZzTyX+PMpp2yrVdnVLntGaXr5O40Pn5aOBfGWb2Xz/RZdzAlgFIQK1W23QiF1xDKxtIu8dlNTc/auyF0YaJrkzQehMnl/JP5FupGAG/4dSDpk8jwEQD7Z0nnU6oVi1p0= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698426983; c=relaxed/simple; bh=wq/N9hvd0E+6DPzsHBPFBrBfZ4dEuTjOd8AmtTjtez0=; h=DKIM-Signature:Date:From:To:Subject:Message-ID:MIME-Version; b=LonPOC7K4sgOHGIYNdfsWMEfIijLsg7o01/1btpfarYkBV2mnJKqYKwQofB8MEJTybaxbRZg44qfpI6XO3Nb5QwnZg9EE/3o36XZk743Rum+0wM2cUEmxAxIa+lmTHzNtRgT3o9VoKKORmrVy8wcmnyfkgHLjnm7i+qmX0XLTVo= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x12f.google.com with SMTP id 2adb3069b0e04-507a62d4788so3590497e87.0 for ; Fri, 27 Oct 2023 10:16:21 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1698426977; x=1699031777; darn=gcc.gnu.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:from:to:cc:subject:date :message-id:reply-to; bh=taDe/ZAS5KqZuJ/VJhICm042b5IU9CvJqfbLkUQBrbI=; b=X0Cib1HVDrxz16Qt51U1GIMD3k0etC/8P7csEatEXjTTAu0fTl7yekxlTYky0t0xb8 su4TqEdZBbE6cx/7uK45GKkfzNYdqIgcV06LCkT+g4AmT/XZQY69QD3AxCteKVaCmhY4 X/KlylvlUMKLYXtVwT6/mbB+wOdaGAqmS7fLol11nFAEyfWrF1AKGVLH6PoN0mmKK4i5 e8Gu1A27R2MN4yhsCs25NtKdBZPYJrSA9Yh95FDYhNJl9h/aLVzgp182+gj0oL7F/wej KM8Yi2KN4znD4UzO0hNVtZGu4fy95tKkoj0H8fJxWQfoRv8eIoWvbNJhA1engyNMOGm8 SV5w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698426977; x=1699031777; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:subject:cc:to:from:date:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=taDe/ZAS5KqZuJ/VJhICm042b5IU9CvJqfbLkUQBrbI=; b=JkPmUMr0UmgXKD0CztYrjiCSeZoequLhV3vThPxU+LC3Dea5Myg8hSyTSvT92Ctnv0 nKpUXWZ2eSUVVG+nbWe9Mjew+Vy0E76t+Ao0UvTeHbtqBTbdYDBFZ0MZ/bL48RsQOtxB Ankj2d5pdTP3yl+vrrBPIaVQdYH7EqqoUsSi+QlTFJNolMbsIMNrK597VJHQQEGz5aWQ PhR76bkesfOka29Ad0YuL9Qa6EkLH5yoS8D+0eUfsPpvCaO8snycqFELZpXLePpZis4Y +0FqSe8cqVftJEoO99xubuuPLNzy6L0j4uicJXSc6gLgf+JEt4R2qCZtp4Ci1JNXnWtj WU1A== X-Gm-Message-State: AOJu0YxAkV2hsc/mw+naRJKzGG0yn+nQb/YKw+kf4nyIap60VMpKUkiN 9dvpSjE7Qd7tVC8xyvkbzzk= X-Google-Smtp-Source: AGHT+IGRHxg+c43XGz5+GTXB97FgLXPkomCRzQ6BgctOYNWE6ebrXgqbuI94ZsEe5xtr+HXqMumUtA== X-Received: by 2002:ac2:43b9:0:b0:508:1a2c:46d0 with SMTP id t25-20020ac243b9000000b005081a2c46d0mr2061284lfl.15.1698426977044; Fri, 27 Oct 2023 10:16:17 -0700 (PDT) Received: from nbbrfq.loc ([2001:871:227:b560:2468:ea59:3c5:d50f]) by smtp.gmail.com with ESMTPSA id o27-20020a509b1b000000b0054130b1bc77sm1536981edi.51.2023.10.27.10.16.16 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Oct 2023 10:16:16 -0700 (PDT) Date: Fri, 27 Oct 2023 19:16:12 +0200 From: Bernhard Reutner-Fischer To: Ajit Agarwal Cc: rep.dot.nop@gmail.com, Vineet Gupta , gcc-patches@gcc.gnu.org, Jeff Law , Richard Biener , Segher Boessenkool , Peter Bergner , gnu-toolchain Subject: Re: [PATCH v9 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces Message-ID: <20231027191612.068c67ee@nbbrfq.loc> In-Reply-To: <7379c85b-178c-4196-a929-129052245165@linux.ibm.com> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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 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; } 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.