On 28/10/23 3:55 pm, Ajit Agarwal wrote:
> 
> 
> 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.

The above rearranging code breaks the logic and I have modified 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


 
>>>
>>> I have not done any assembly diff as myself have not cross compiled with 
>>> aarch64.
>>
>> fair enough.

Reply via email to