Re: [PATCH v9 4/4] ree: Improve ree pass for rs6000 target using defined ABI interfaces

[Ajit Agarwal <aagarwa1@linux.ibm.com>]

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