On 24/10/23 1:10 pm, Ajit Agarwal wrote:
> 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.
Made changes in version 13 of the patch with changes since v6.
Thanks & Regards
Ajit
>
>> 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
>>
