On 28/10/23 3:56 pm, Ajit Agarwal wrote:
>
>
> 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,
The above rearranging code breaks the logic. I have implemented as follows.
+/* 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);
+ 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))
+ && dst_mode != GET_MODE (orig_src))
+ {
+ if (!abi_target_promote_function_mode (GET_MODE (orig_src))
+ && REGNO (SET_DEST (set)) != REGNO (orig_src))
+ return false;
+
+ return true;
+ }
+ return false;
+}
Thanks & Regards
Ajit
>> -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.
>>
