From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-x22b.google.com (mail-oi1-x22b.google.com [IPv6:2607:f8b0:4864:20::22b]) by sourceware.org (Postfix) with ESMTPS id 88C943858C66 for ; Mon, 23 Oct 2023 18:32:35 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 88C943858C66 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=rivosinc.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=rivosinc.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 88C943858C66 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::22b ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698085957; cv=none; b=IF2dogfAUwDGKfEWZV8yfv2AI8y0DXo81RYk2Lav1mDZEAIFL5jxmpqvFxtF3aj1H4K87REoNW5c7eV3Cj6LWX2HMchEODA85DR6ej+lmkVlraNpnO7aN8m2VuBh5Nl8IYlojVY5kmlDYuu/WFt1KzyVe0MJ9T1EnW8FWUxN+UM= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1698085957; c=relaxed/simple; bh=pYFwhRA7FfKChXCvPHCiohcQYthFLlvw4nTU7BGbPVI=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=ZRkCpZk8OggPWzZ9lHuCmw3jabrT9oxS9gCV3cBmlgeGgCbW5cmujf99GtNyRfkhnx5dEZMwiN5cENoB6X2PCTyDq2P8CJXcLp+Ex5/80yASxHc/Wejh35LfkFYPtJBgX9/JavtRyCIIGnKOooy5H/8B3koedgMNU06Xz+kVmQE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-oi1-x22b.google.com with SMTP id 5614622812f47-3ae5a014d78so2394268b6e.1 for ; Mon, 23 Oct 2023 11:32:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=rivosinc-com.20230601.gappssmtp.com; s=20230601; t=1698085955; x=1698690755; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=DztT/Q7cHrzMx/pAsE+0TcfCgRSJ1Did0M7DFoz0qKU=; b=ZxxSb55VXIIpgnDV9Qx2pMdc1q0fXRySKXZNk8027sxyJcmmMR3bhrvXL/xX99Ap93 WTpDzoEAMXjd58h6vPCgC/mZtGBZPv2b+Vcta0SwF2T8rTU7+Gon/2lS4kJbllx4M6iP JCIAY53KwlEVvZiCEkWBf5hq8jzBe8iexxR5XaCAS+ukaIEw7qXhUPcXupI0m52LPBLu 1bg0mI+3sFeO36VIOqw4DxznVPOKN8Ua8pE1K4J0h8Br9rt/6lNfgCykLF3gRCc+r/Hi VkCNuGYGt1Aa0pbs6uu25wGUNBx+65+QDAxmFw4T27UJO4SxoLAZ/PqvdXIUD49q3t6S dPGQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1698085955; x=1698690755; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=DztT/Q7cHrzMx/pAsE+0TcfCgRSJ1Did0M7DFoz0qKU=; b=TXBRx2vxCx/hoPR3BRml4VAd9t411S0/I4gRQ+/IeI5wj+O8bzh7Zl60WDdPwG6DvH wnaFwGxnui9Z7kezbHrghwzch6ZkvYQ1hBAbq8UfwxBkm1/3unDdm9FeLCUONnNuuwuk FWPK8Btbpp5LlpH+hcV5Zr85gvCM4YopE0n0DH+G2JE4EbDFV6tR1FaPhUpvV1vRDpbw iJHsqWGeWZi9uHoHRiv3SmDnHmdI8cys6QqrCFGRn3H8PrbeH3OkMkGWVr1VtfORDpIv 6LdGVrUN7IY0G6y8krUhgIefMCX1VtGU0K+KnlQtjpkyo8a9FdO4eUdjwY4lk2WvIhQw BKJg== X-Gm-Message-State: AOJu0Yw7puIroox0DV3Pcnh4T6lyWyRYl7dT47gjg2wi/dKnlLHBJdaS 8E49hRPrwmfRPGMchAFPktj0BA== X-Google-Smtp-Source: AGHT+IFxB5Q+KmDAaigrkvD4KRj3ADGvZBWLsy2EpZJz7XUd/BvDhXfWQJ2gxdnOjkkdm8uNLUyxPQ== X-Received: by 2002:a05:6808:181d:b0:3a8:8aa8:a4d7 with SMTP id bh29-20020a056808181d00b003a88aa8a4d7mr5246998oib.3.1698085953444; Mon, 23 Oct 2023 11:32:33 -0700 (PDT) Received: from [192.168.50.117] (c-98-210-197-24.hsd1.ca.comcast.net. [98.210.197.24]) by smtp.gmail.com with ESMTPSA id n6-20020a0568080a0600b003ae51628725sm1578992oij.13.2023.10.23.11.32.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 23 Oct 2023 11:32:33 -0700 (PDT) Message-ID: <6c2c1b77-b341-4abc-b572-e402d0d14d8b@rivosinc.com> Date: Mon, 23 Oct 2023 11:32:31 -0700 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 To: Ajit Agarwal , 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> From: Vineet Gupta In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-5.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,KAM_SHORT,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 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. 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. 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. I do have some technical comments which I'll follow up with later. 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. -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