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....@gmail.com wrote: >>> On 21 October 2023 01:56:16 CEST, Vineet Gupta <vine...@rivosinc.com> 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 >
