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

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

On 25/10/23 2:06 am, rep.dot.nop@gmail.com wrote:
> On 24 October 2023 09:36:22 CEST, Ajit Agarwal <aagarwa1@linux.ibm.com> wrote:
>> Hello Bernhard:
>>
>> On 23/10/23 7:40 pm, Bernhard Reutner-Fischer wrote:
>>> On Mon, 23 Oct 2023 12:16:18 +0530
>>> Ajit Agarwal <aagarwa1@linux.ibm.com> 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.
>>>
>>> s/satisified/satisfied/
>>>
>>
>> I will fix this.
> 
> thanks!
> 
>>
>>>>> 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 might have not completely understood the subtile intricacies of RTL re-entrancy, it seems?
> 

Here is the abi interface where I CSE'D and got a mail from automated regressions run that aarch64
test fails.

static inline bool
abi_extension_candidate_return_reg_p (int regno)
{
  if (targetm.calls.function_value_regno_p (regno))
    return true;

  return false;
}

+static inline bool
+abi_extension_candidate_return_reg_p (int regno)
+{
+  return targetm.calls.function_value_regno_p (regno);
+}


I have not done any assembly diff as myself have not cross compiled with aarch64.
Reverting above CSE the tests passes with automatically regression runs and build with linaro.
Linaro runs the tests with every patch we submit in gcc-patches and if there is any fail they 
report error.

Reverting CSE the Linaro tests passes.

Thanks & Regards
Ajit
> thanks
> 
>>>>>   
>>>>>>> +/* 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)))  
>>>>>>> +    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).
>>>
>>> thanks
>