[PATCH] ira-color: fix allocno_priority_compare_func for qsort (PR 82395)

[PATCH] ira-color: fix allocno_priority_compare_func for qsort (PR 82395)

[Alexander Monakov]

Hello,

In ira-color.c, qsort comparator allocno_priority_compare_func lacks anti-
commutativity and can indicate A < B < A if boths allocnos satisfy
non_spilled_static_chain_regno_p.  It should fall down to following
sub-comparisons in that case.

There is another issue: the comment doesn't match the code.  We are sorting
allocnos by priority, higher first, and the comment says that allocnos
corresponding to static chain pointer register should go first. However,
the code implements the opposite ordering.

I think the comment documents the intended behavior and the code is wrong.
Thus, the patch changes comparison value to match the comment.

A similar issue was present in lra-assigns.c and was fixed by this patch:
https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00893.html

Bootstrapped and regtested on x86-64, OK for trunk?

Thanks.
Alexander

	PR rtl-optimization/82395
	* ira-color.c (allocno_priority_compare_func): Fix comparison step
	based on non_spilled_static_chain_regno_p.

diff --git a/gcc/ira-color.c b/gcc/ira-color.c
index 22fdb88274d..31a4a8074d1 100644
--- a/gcc/ira-color.c
+++ b/gcc/ira-color.c
@@ -3005,14 +3005,13 @@ allocno_priority_compare_func (const void *v1p, const void *v2p)
 {
   ira_allocno_t a1 = *(const ira_allocno_t *) v1p;
   ira_allocno_t a2 = *(const ira_allocno_t *) v2p;
-  int pri1, pri2;
+  int pri1, pri2, diff;
 
   /* Assign hard reg to static chain pointer pseudo first when
      non-local goto is used.  */
-  if (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a1)))
-    return 1;
-  else if (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a2)))
-    return -1;
+  if ((diff = (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a2))
+	       - non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a1)))) != 0)
+    return diff;
   pri1 = allocno_priorities[ALLOCNO_NUM (a1)];
   pri2 = allocno_priorities[ALLOCNO_NUM (a2)];
   if (pri2 != pri1)

Re: [PATCH] ira-color: fix allocno_priority_compare_func for qsort (PR 82395)

[Alexander Monakov]

Ping.

On Thu, 5 Oct 2017, Alexander Monakov wrote:
> In ira-color.c, qsort comparator allocno_priority_compare_func lacks anti-
> commutativity and can indicate A < B < A if boths allocnos satisfy
> non_spilled_static_chain_regno_p.  It should fall down to following
> sub-comparisons in that case.
> 
> There is another issue: the comment doesn't match the code.  We are sorting
> allocnos by priority, higher first, and the comment says that allocnos
> corresponding to static chain pointer register should go first. However,
> the code implements the opposite ordering.
> 
> I think the comment documents the intended behavior and the code is wrong.
> Thus, the patch changes comparison value to match the comment.
> 
> A similar issue was present in lra-assigns.c and was fixed by this patch:
> https://gcc.gnu.org/ml/gcc-patches/2017-07/msg00893.html
> 
> Bootstrapped and regtested on x86-64, OK for trunk?
> 
> Thanks.
> Alexander
> 
> 	PR rtl-optimization/82395
> 	* ira-color.c (allocno_priority_compare_func): Fix comparison step
> 	based on non_spilled_static_chain_regno_p.
> 
> diff --git a/gcc/ira-color.c b/gcc/ira-color.c
> index 22fdb88274d..31a4a8074d1 100644
> --- a/gcc/ira-color.c
> +++ b/gcc/ira-color.c
> @@ -3005,14 +3005,13 @@ allocno_priority_compare_func (const void *v1p, const void *v2p)
>  {
>    ira_allocno_t a1 = *(const ira_allocno_t *) v1p;
>    ira_allocno_t a2 = *(const ira_allocno_t *) v2p;
> -  int pri1, pri2;
> +  int pri1, pri2, diff;
>  
>    /* Assign hard reg to static chain pointer pseudo first when
>       non-local goto is used.  */
> -  if (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a1)))
> -    return 1;
> -  else if (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a2)))
> -    return -1;
> +  if ((diff = (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a2))
> +	       - non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a1)))) != 0)
> +    return diff;
>    pri1 = allocno_priorities[ALLOCNO_NUM (a1)];
>    pri2 = allocno_priorities[ALLOCNO_NUM (a2)];
>    if (pri2 != pri1)

Re: [PATCH] ira-color: fix allocno_priority_compare_func for qsort (PR 82395)

[Vladimir Makarov]

On 10/19/2017 06:37 AM, Alexander Monakov wrote:
> Ping.
>
> On Thu, 5 Oct 2017, Alexander Monakov wrote:
> Bootstrapped and regtested on x86-64, OK for trunk?
>
OK.  Alexander, sorry for the delay with the answer and thank you for 
finding and fixing the problem.
> 	PR rtl-optimization/82395
> 	* ira-color.c (allocno_priority_compare_func): Fix comparison step
> 	based on non_spilled_static_chain_regno_p.
>
> diff --git a/gcc/ira-color.c b/gcc/ira-color.c
> index 22fdb88274d..31a4a8074d1 100644
> --- a/gcc/ira-color.c
> +++ b/gcc/ira-color.c
> @@ -3005,14 +3005,13 @@ allocno_priority_compare_func (const void *v1p, const void *v2p)
>   {
>     ira_allocno_t a1 = *(const ira_allocno_t *) v1p;
>     ira_allocno_t a2 = *(const ira_allocno_t *) v2p;
> -  int pri1, pri2;
> +  int pri1, pri2, diff;
>   
>     /* Assign hard reg to static chain pointer pseudo first when
>        non-local goto is used.  */
> -  if (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a1)))
> -    return 1;
> -  else if (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a2)))
> -    return -1;
> +  if ((diff = (non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a2))
> +	       - non_spilled_static_chain_regno_p (ALLOCNO_REGNO (a1)))) != 0)
> +    return diff;
>     pri1 = allocno_priorities[ALLOCNO_NUM (a1)];
>     pri2 = allocno_priorities[ALLOCNO_NUM (a2)];
>     if (pri2 != pri1)