On 27/10/23 10:46 pm, Bernhard Reutner-Fischer wrote:
> On Wed, 25 Oct 2023 16:41:07 +0530
> Ajit Agarwal <aagar...@linux.ibm.com> wrote:
>
>> On 25/10/23 2:19 am, Vineet Gupta wrote:
>>> On 10/24/23 13:36, rep.dot....@gmail.com wrote:
>>>>>>>> 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 was meaning to ask this before, but what exactly is the CSE issue,
>>> manually or whatever.
>
> If nothing else it would hopefully improve the readability.
>
>>>
>> Here is the abi interface where I CSE'D and got a mail from automated
>> regressions run that aarch64
>> test fails.
>
> We already concluded that this failure was obviously a hiccup on the
> testers, no problem.
Thanks.
>
>> +static inline bool
>> +abi_extension_candidate_return_reg_p (int regno)
>> +{
>> + return targetm.calls.function_value_regno_p (regno);
>> +}
>
> But i was referring to abi_extension_candidate_p :)
>
> your v13 looks like this:
>
> +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 (REGNO (orig_src)))
> + return false;
> +
> + /* Return FALSE if mode of destination and source is same. */
> + 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);
> +
> + /* Return FALSE if promote is false and REGNO of source and destination
> + is different. */
> + if (!promote_p && REGNO (SET_DEST (set)) != REGNO (orig_src))
> + return false;
> +
> + return true;
> +}
>
> and i suppose it would be easier to read if phrased something like
>
> static bool
> abi_extension_candidate_p (rtx_insn *insn)
> {
> rtx set = single_set (insn);
> rtx orig_src = XEXP (SET_SRC (set), 0);
> unsigned int src_regno = REGNO (orig_src);
>
> /* Not a function argument reg or is a function values return reg. */
> if (!FUNCTION_ARG_REGNO_P (src_regno)
> || abi_extension_candidate_return_reg_p (src_regno))
> return false;
>
> rtx dst = SET_DST (set);
> machine_mode src_mode = GET_MODE (orig_src);
>
> /* Return FALSE if mode of destination and source is the same. */
> if (GET_MODE (dst) == src_mode)
> return false;
>
> /* Return FALSE if the FIX THE COMMENT and REGNO of source and destination
> is different. */
> if (!abi_target_promote_function_mode_p (src_mode)
> && REGNO (dst) != src_regno)
> return false;
>
> return true;
> }
>
> so no, that's not exactly better.
>
> Maybe just do what the function comment says (i did not check the "not
> promoted" part, but you get the idea):
>
> ^L
>
> /* Return TRUE if
> reg source operand is argument register and not return register,
> mode of source and destination operand are different,
> if not promoted REGNO of source and destination operand are the same. */
> static bool
> abi_extension_candidate_p (rtx_insn *insn)
> {
> rtx set = single_set (insn);
> rtx orig_src = XEXP (SET_SRC (set), 0);
>
> if (FUNCTION_ARG_REGNO_P (REGNO (orig_src))
> && !abi_extension_candidate_return_reg_p (REGNO (orig_src))
> && GET_MODE (SET_DST (set)) != GET_MODE (orig_src)
> && abi_target_promote_function_mode_p (GET_MODE (orig_src))
> && REGNO (SET_DST (set)) == REGNO (orig_src))
> return true;
>
> return false;
> }
>
> I think this is much easier to actually read (and that's why good
> function comments are important). In the end it's not important and
> just personal preference.
> Either way, I did not check the plausibility of the logic therein.
>
>>
Addressed in V15 of the patch.
>>
>> I have not done any assembly diff as myself have not cross compiled with
>> aarch64.
>
> fair enough.
