On 28/10/23 4:09 am, Vineet Gupta wrote:
>
>
> On 10/27/23 10:16, 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.
>>
>>> +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;
>> }
>
> This may have been my doing as I asked to split out the logic as some of the
> conditions merit more commentary.
> e.g. why does the mode need to be same
> But granted this is the usual coding style in gcc and the extra comments
> could still be added before the big if
>
Addressed in V15 of the patch,
> -Vineet
>
>>
>> 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.
>>
>>>
>>> I have not done any assembly diff as myself have not cross compiled with
>>> aarch64.
>> fair enough.
>
