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

[Alexander Monakov <amonakov at ispras dot ru>]

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)