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 <vmaka...@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;
> +}
