[pushed,LRA,PR110372] : Refine reload pseudo class

The following patch solves

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110372

The patch was successfully bootstrapped and tested on x86-64.

Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
> The following patch solves
>
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110372
>
> The patch was successfully bootstrapped and tested on x86-64.
>
> commit 1f7e5a7b91862b999aab88ee0319052aaf00f0f1
> Author: Vladimir N. Makarov <vmakarov@redhat.com>
> Date:   Fri Jul 7 09:53:38 2023 -0400
>
>     LRA: Refine reload pseudo class
>     
>     For given testcase a reload pseudo happened to occur only in reload
>     insns created on one constraint sub-pass.  Therefore its initial class
>     (ALL_REGS) was not refined and the reload insns were not processed on
>     the next constraint sub-passes.  This resulted into the wrong insn.
>     
>             PR rtl-optimization/110372
>     
>     gcc/ChangeLog:
>     
>             * lra-assigns.cc (assign_by_spills): Add reload insns involving
>             reload pseudos with non-refined class to be processed on the next
>             sub-pass.
>             * lra-constraints.cc (enough_allocatable_hard_regs_p): New func.
>             (in_class_p): Use it.
>             (print_curr_insn_alt): New func.
>             (process_alt_operands): Use it.  Improve debug info.
>             (curr_insn_transform): Use print_curr_insn_alt.  Refine reload
>             pseudo class if it is not refined yet.
>     
>     gcc/testsuite/ChangeLog:
>     
>             * gcc.target/i386/pr110372.c: New.
>
> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
> index 73fbef29912..2f95121df06 100644
> --- a/gcc/lra-assigns.cc
> +++ b/gcc/lra-assigns.cc
> @@ -1443,10 +1443,11 @@ assign_by_spills (void)
>  		 pass.  Indicate that it is no longer spilled.  */
>  	      bitmap_clear_bit (&all_spilled_pseudos, regno);
>  	      assign_hard_regno (hard_regno, regno);
> -	      if (! reload_p)
> -		/* As non-reload pseudo assignment is changed we
> -		   should reconsider insns referring for the
> -		   pseudo.  */
> +	      if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS)

Is this test meaningful on all targets?  We have some for which
GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can
be a valid allocation class.

Thanks,
Richard

> +		/* As non-reload pseudo assignment is changed we should
> +		   reconsider insns referring for the pseudo.  Do the same if a
> +		   reload pseudo did not refine its class which can happens
> +		   when the pseudo occurs only in reload insns.  */
>  		bitmap_set_bit (&changed_pseudo_bitmap, regno);
>  	    }
>  	}
> diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
> index 4dc2d70c402..123ff662cbc 100644
> --- a/gcc/lra-constraints.cc
> +++ b/gcc/lra-constraints.cc
> @@ -233,6 +233,34 @@ get_reg_class (int regno)
>    return NO_REGS;
>  }
>  
> +/* Return true if REG_CLASS has enough allocatable hard regs to keep value of
> +   REG_MODE.  */
> +static bool
> +enough_allocatable_hard_regs_p (enum reg_class reg_class,
> +				enum machine_mode reg_mode)
> +{
> +  int i, j, hard_regno, class_size, nregs;
> +  
> +  if (hard_reg_set_subset_p (reg_class_contents[reg_class], lra_no_alloc_regs))
> +    return false;
> +  class_size = ira_class_hard_regs_num[reg_class];
> +  for (i = 0; i < class_size; i++)
> +    {
> +      hard_regno = ira_class_hard_regs[reg_class][i];
> +      nregs = hard_regno_nregs (hard_regno, reg_mode);
> +      if (nregs == 1)
> +	return true;
> +      for (j = 0; j < nregs; j++)
> +	if (TEST_HARD_REG_BIT (lra_no_alloc_regs, hard_regno + j)
> +	    || ! TEST_HARD_REG_BIT (reg_class_contents[reg_class],
> +				    hard_regno + j))
> +	  break;
> +      if (j >= nregs)
> +	return true;
> +    }
> +  return false;
> +}
> +
>  /* Return true if REG satisfies (or will satisfy) reg class constraint
>     CL.  Use elimination first if REG is a hard register.  If REG is a
>     reload pseudo created by this constraints pass, assume that it will
> @@ -252,7 +280,6 @@ in_class_p (rtx reg, enum reg_class cl, enum reg_class *new_class,
>    enum reg_class rclass, common_class;
>    machine_mode reg_mode;
>    rtx src;
> -  int class_size, hard_regno, nregs, i, j;
>    int regno = REGNO (reg);
>  
>    if (new_class != NULL)
> @@ -291,26 +318,7 @@ in_class_p (rtx reg, enum reg_class cl, enum reg_class *new_class,
>        common_class = ira_reg_class_subset[rclass][cl];
>        if (new_class != NULL)
>  	*new_class = common_class;
> -      if (hard_reg_set_subset_p (reg_class_contents[common_class],
> -				 lra_no_alloc_regs))
> -	return false;
> -      /* Check that there are enough allocatable regs.  */
> -      class_size = ira_class_hard_regs_num[common_class];
> -      for (i = 0; i < class_size; i++)
> -	{
> -	  hard_regno = ira_class_hard_regs[common_class][i];
> -	  nregs = hard_regno_nregs (hard_regno, reg_mode);
> -	  if (nregs == 1)
> -	    return true;
> -	  for (j = 0; j < nregs; j++)
> -	    if (TEST_HARD_REG_BIT (lra_no_alloc_regs, hard_regno + j)
> -		|| ! TEST_HARD_REG_BIT (reg_class_contents[common_class],
> -					hard_regno + j))
> -	      break;
> -	  if (j >= nregs)
> -	    return true;
> -	}
> -      return false;
> +      return enough_allocatable_hard_regs_p (common_class, reg_mode);
>      }
>  }
>  
> @@ -2046,6 +2054,23 @@ update_and_check_small_class_inputs (int nop, int nalt,
>    return false;
>  }
>  
> +/* Print operand constraints for alternative ALT_NUMBER of the current
> +   insn.  */
> +static void
> +print_curr_insn_alt (int alt_number)
> +{
> +  for (int i = 0; i < curr_static_id->n_operands; i++)
> +    {
> +      const char *p = (curr_static_id->operand_alternative
> +		       [alt_number * curr_static_id->n_operands + i].constraint);
> +      if (*p == '\0')
> +	continue;
> +      fprintf (lra_dump_file, "  (%d) ", i);
> +      for (; *p != '\0' && *p != ',' && *p != '#'; p++)
> +	fputc (*p, lra_dump_file);
> +    }
> +}
> +
>  /* Major function to choose the current insn alternative and what
>     operands should be reloaded and how.	 If ONLY_ALTERNATIVE is not
>     negative we should consider only this alternative.  Return false if
> @@ -2145,6 +2170,14 @@ process_alt_operands (int only_alternative)
>        if (!TEST_BIT (preferred, nalt))
>  	continue;
>  
> +      if (lra_dump_file != NULL)
> +	{
> +	  fprintf (lra_dump_file, "         Considering alt=%d of insn %d: ",
> +		   nalt, INSN_UID (curr_insn));
> +	  print_curr_insn_alt (nalt);
> +	  fprintf (lra_dump_file, "\n");
> +	}
> +      
>        bool matching_early_clobber[MAX_RECOG_OPERANDS];
>        curr_small_class_check++;
>        overall = losers = addr_losers = 0;
> @@ -2671,8 +2704,7 @@ process_alt_operands (int only_alternative)
>  		{
>  		  if (lra_dump_file != NULL)
>  		    fprintf (lra_dump_file,
> -			     "            alt=%d: Bad operand -- refuse\n",
> -			     nalt);
> +			     "            Bad operand -- refuse\n");
>  		  goto fail;
>  		}
>  
> @@ -2701,8 +2733,7 @@ process_alt_operands (int only_alternative)
>  			{
>  			  if (lra_dump_file != NULL)
>  			    fprintf (lra_dump_file,
> -				     "            alt=%d: Wrong mode -- refuse\n",
> -				     nalt);
> +				     "            Wrong mode -- refuse\n");
>  			  goto fail;
>  			}
>  		    }
> @@ -2769,8 +2800,7 @@ process_alt_operands (int only_alternative)
>  		      if (lra_dump_file != NULL)
>  			fprintf
>  			  (lra_dump_file,
> -			   "            alt=%d: Strict low subreg reload -- refuse\n",
> -			   nalt);
> +			   "            Strict low subreg reload -- refuse\n");
>  		      goto fail;
>  		    }
>  		  losers++;
> @@ -2832,8 +2862,7 @@ process_alt_operands (int only_alternative)
>  		  if (lra_dump_file != NULL)
>  		    fprintf
>  		      (lra_dump_file,
> -		       "            alt=%d: No input/output reload -- refuse\n",
> -		       nalt);
> +		       "            No input/output reload -- refuse\n");
>  		  goto fail;
>  		}
>  
> @@ -2860,9 +2889,9 @@ process_alt_operands (int only_alternative)
>  		{
>  		  if (lra_dump_file != NULL)
>  		    fprintf (lra_dump_file,
> -			     "            alt=%d: reload pseudo for op %d "
> +			     "            reload pseudo for op %d "
>  			     "cannot hold the mode value -- refuse\n",
> -			     nalt, nop);
> +			     nop);
>  		  goto fail;
>  		}
>  
> @@ -2989,7 +3018,7 @@ process_alt_operands (int only_alternative)
>  	      
>  	      /* If reload requires moving value through secondary
>  		 memory, it will need one more insn at least.  */
> -	      if (this_alternative != NO_REGS 
> +	      if (this_alternative != NO_REGS
>  		  && REG_P (op) && (cl = get_reg_class (REGNO (op))) != NO_REGS
>  		  && ((curr_static_id->operand[nop].type != OP_OUT
>  		       && targetm.secondary_memory_needed (GET_MODE (op), cl,
> @@ -3046,8 +3075,8 @@ process_alt_operands (int only_alternative)
>              {
>                if (lra_dump_file != NULL)
>  		fprintf (lra_dump_file,
> -			 "            alt=%d,overall=%d,losers=%d -- refuse\n",
> -			 nalt, overall, losers);
> +			 "            overall=%d,losers=%d -- refuse\n",
> +			 overall, losers);
>                goto fail;
>              }
>  
> @@ -3056,8 +3085,7 @@ process_alt_operands (int only_alternative)
>  	    {
>  	      if (lra_dump_file != NULL)
>  		fprintf (lra_dump_file,
> -			 "            alt=%d, not enough small class regs -- refuse\n",
> -			 nalt);
> +			 "            not enough small class regs -- refuse\n");
>  	      goto fail;
>  	    }
>  	  curr_alt[nop] = this_alternative;
> @@ -3238,8 +3266,8 @@ process_alt_operands (int only_alternative)
>  	  overall += LRA_LOSER_COST_FACTOR - 1;
>  	}
>        if (lra_dump_file != NULL)
> -	fprintf (lra_dump_file, "          alt=%d,overall=%d,losers=%d,rld_nregs=%d\n",
> -		 nalt, overall, losers, reload_nregs);
> +	fprintf (lra_dump_file, "          overall=%d,losers=%d,rld_nregs=%d\n",
> +		 overall, losers, reload_nregs);
>  
>        /* If this alternative can be made to work by reloading, and it
>  	 needs less reloading than the others checked so far, record
> @@ -4355,18 +4383,9 @@ curr_insn_transform (bool check_only_p)
>      {
>        const char *p;
>  
> -      fprintf (lra_dump_file, "	 Choosing alt %d in insn %u:",
> +      fprintf (lra_dump_file, "      Choosing alt %d in insn %u:",
>  	       goal_alt_number, INSN_UID (curr_insn));
> -      for (i = 0; i < n_operands; i++)
> -	{
> -	  p = (curr_static_id->operand_alternative
> -	       [goal_alt_number * n_operands + i].constraint);
> -	  if (*p == '\0')
> -	    continue;
> -	  fprintf (lra_dump_file, "  (%d) ", i);
> -	  for (; *p != '\0' && *p != ',' && *p != '#'; p++)
> -	    fputc (*p, lra_dump_file);
> -	}
> +      print_curr_insn_alt (goal_alt_number);
>        if (INSN_CODE (curr_insn) >= 0
>            && (p = get_insn_name (INSN_CODE (curr_insn))) != NULL)
>          fprintf (lra_dump_file, " {%s}", p);
> @@ -4564,7 +4583,22 @@ curr_insn_transform (bool check_only_p)
>  	      continue;
>  	    }
>  	  else
> -	    continue;
> +	    {
> +	      enum reg_class rclass, common_class;
> +	      
> +	      if (REG_P (op) && goal_alt[i] != NO_REGS
> +		  && (regno = REGNO (op)) >= new_regno_start
> +		  && (rclass = get_reg_class (regno)) == ALL_REGS
> +		  && ((common_class = ira_reg_class_subset[rclass][goal_alt[i]])
> +		      != NO_REGS)
> +		  && common_class != ALL_REGS
> +		  && enough_allocatable_hard_regs_p (common_class,
> +						     GET_MODE (op)))
> +		/* Refine reload pseudo class from chosen alternative
> +		   constraint.  */
> +		lra_change_class (regno, common_class, "      Change to", true);
> +	      continue;
> +	    }
>  	}
>  
>        /* Operands that match previous ones have already been handled.  */
> @@ -5249,7 +5283,7 @@ lra_constraints (bool first_p)
>  		   the equiv.  We could update the equiv insns after
>  		   transformations including an equiv insn deletion
>  		   but it is not worthy as such cases are extremely
> -		   rare.  */ 
> +		   rare.  */
>  		|| contains_deleted_insn_p (ira_reg_equiv[i].init_insns)
>  		/* If it is not a reverse equivalence, we check that a
>  		   pseudo in rhs of the init insn is not dying in the
> diff --git a/gcc/testsuite/gcc.target/i386/pr110372.c b/gcc/testsuite/gcc.target/i386/pr110372.c
> new file mode 100644
> index 00000000000..dd7d76a36f3
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/i386/pr110372.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile { target { ! ia32 } } } */
> +/* { dg-options "-O -mno-sse2" } */
> +
> +typedef char __attribute__((__vector_size__ (16))) U;
> +typedef int __attribute__((__vector_size__ (16))) V;
> +
> +V v;
> +
> +U
> +foo0 (U u)
> +{
> +  v *= (V) u & 4;
> +  return u;
> +}

On 7/12/23 06:07, Richard Sandiford wrote:
> Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
>> index 73fbef29912..2f95121df06 100644
>> --- a/gcc/lra-assigns.cc
>> +++ b/gcc/lra-assigns.cc
>> @@ -1443,10 +1443,11 @@ assign_by_spills (void)
>>   		 pass.  Indicate that it is no longer spilled.  */
>>   	      bitmap_clear_bit (&all_spilled_pseudos, regno);
>>   	      assign_hard_regno (hard_regno, regno);
>> -	      if (! reload_p)
>> -		/* As non-reload pseudo assignment is changed we
>> -		   should reconsider insns referring for the
>> -		   pseudo.  */
>> +	      if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS)
> Is this test meaningful on all targets?  We have some for which
> GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can
> be a valid allocation class.
>
Richard, thank you for the question.

As I remember nvptx does not use IRA/LRA.

I don't think it is a problem.  For targets with GENERAL_REGS == 
ALL_REGS, it only results in one more insn processing on the next 
constraint sub-pass.

I could do more accurate solution but it would need introducing new data 
(flags) for pseudos which I'd like to avoid.

Vladimir Makarov <vmakarov@redhat.com> writes:
> On 7/12/23 06:07, Richard Sandiford wrote:
>> Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
>>> index 73fbef29912..2f95121df06 100644
>>> --- a/gcc/lra-assigns.cc
>>> +++ b/gcc/lra-assigns.cc
>>> @@ -1443,10 +1443,11 @@ assign_by_spills (void)
>>>   		 pass.  Indicate that it is no longer spilled.  */
>>>   	      bitmap_clear_bit (&all_spilled_pseudos, regno);
>>>   	      assign_hard_regno (hard_regno, regno);
>>> -	      if (! reload_p)
>>> -		/* As non-reload pseudo assignment is changed we
>>> -		   should reconsider insns referring for the
>>> -		   pseudo.  */
>>> +	      if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS)
>> Is this test meaningful on all targets?  We have some for which
>> GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can
>> be a valid allocation class.
>>
> Richard, thank you for the question.
>
> As I remember nvptx does not use IRA/LRA.
>
> I don't think it is a problem.  For targets with GENERAL_REGS == 
> ALL_REGS, it only results in one more insn processing on the next 
> constraint sub-pass.

Ah, ok, thanks.  If there's no risk of cycling then I agree it
doesn't matter.

Richard

> I could do more accurate solution but it would need introducing new data 
> (flags) for pseudos which I'd like to avoid.

On 7/12/23 12:22, Richard Sandiford wrote:
> Vladimir Makarov <vmakarov@redhat.com> writes:
>> On 7/12/23 06:07, Richard Sandiford wrote:
>>> Vladimir Makarov via Gcc-patches <gcc-patches@gcc.gnu.org> writes:
>>>> diff --git a/gcc/lra-assigns.cc b/gcc/lra-assigns.cc
>>>> index 73fbef29912..2f95121df06 100644
>>>> --- a/gcc/lra-assigns.cc
>>>> +++ b/gcc/lra-assigns.cc
>>>> @@ -1443,10 +1443,11 @@ assign_by_spills (void)
>>>>    		 pass.  Indicate that it is no longer spilled.  */
>>>>    	      bitmap_clear_bit (&all_spilled_pseudos, regno);
>>>>    	      assign_hard_regno (hard_regno, regno);
>>>> -	      if (! reload_p)
>>>> -		/* As non-reload pseudo assignment is changed we
>>>> -		   should reconsider insns referring for the
>>>> -		   pseudo.  */
>>>> +	      if (! reload_p || regno_allocno_class_array[regno] == ALL_REGS)
>>> Is this test meaningful on all targets?  We have some for which
>>> GENERAL_REGS == ALL_REGS (e.g. nios2 and nvptx), so ALL_REGS can
>>> be a valid allocation class.
>>>
>> Richard, thank you for the question.
>>
>> As I remember nvptx does not use IRA/LRA.
>>
>> I don't think it is a problem.  For targets with GENERAL_REGS ==
>> ALL_REGS, it only results in one more insn processing on the next
>> constraint sub-pass.
> Ah, ok, thanks.  If there's no risk of cycling then I agree it
> doesn't matter.
No. There is no additional risk of cycling as insn processing only 
starts after assigning hard reg to the reload pseudo and it can happens 
only once for the reload pseudo before spilling sub-pass.