Re: [PATCH] [lra] force reg update after spilling to memory [PR120424]

[Alexandre Oliva]

On May 27, 2025, Vladimir Makarov <vmaka...@redhat.com> wrote:

> This is a good explanation of the patch and the bug and the affected
> code seem very old (from the first commit of LRA). So thank you very 
> much for working on and fixing this PR.

Thanks for the review.

Jeff points out gcc.target/avr/torture/pr118591-1.c now fails to compile
at all nonzero optimization levels on avr-none.

The problem seems to be that, even after fp2sp is disabled, but before
update_reg_eliminate gets a chance to run again, we spill some more
pseudos and end up using the fp2sp elimination.

I noticed that while investigating PR120424, and I had the patchlet
below at some point, but I ended up concluding that it was not
necessary, because the eliminations are reversible, and I figured that
stopping applying them half-way would make for trouble reversing them.
This tweak avoids the problem, but it's probably not the way to go
for the reasons above.

diff --git a/gcc/lra-eliminations.cc b/gcc/lra-eliminations.cc
index bb708b007a4..36c5365f818 100644
--- a/gcc/lra-eliminations.cc
+++ b/gcc/lra-eliminations.cc
@@ -263,7 +263,7 @@ get_elimination (rtx reg)
   if ((hard_regno = REGNO (reg)) < 0 || hard_regno >= FIRST_PSEUDO_REGISTER)
     return NULL;
   if ((ep = elimination_map[hard_regno]) != NULL)
-    return ep->from_rtx != reg ? NULL : ep;
+    return ep->from_rtx != reg || !ep->can_eliminate ? NULL : ep;
   poly_int64 offset = self_elim_offsets[hard_regno];
   if (known_eq (offset, 0))
     return NULL;

Now, since lra_update_fp2sp_elimination checks that
!elimination_fp2sp_occured_p, we *could* disable the fp2sp elimination,
if it's selected, right away, so that it is not applied after we've
disabled it, and then we don't have to worry about disabling it half-way
or reversing it later.  Like the patchlet above, the one below also
cures the avr regression.  Does this strike you as the way to go, or can
you envision better ways to approach this?


--- a/gcc/lra-eliminations.cc
+++ b/gcc/lra-eliminations.cc
@@ -1415,6 +1415,14 @@
   if (frame_pointer_needed || !targetm.frame_pointer_required ())
     return 0;
   gcc_assert (!elimination_fp2sp_occured_p);
+  ep = elimination_map[FRAME_POINTER_REGNUM];
+  if (ep->to == STACK_POINTER_REGNUM)
+    {
+      elimination_map[FRAME_POINTER_REGNUM] = NULL;
+      setup_can_eliminate (ep, false);
+    }
+  else
+    ep = NULL;
   if (lra_dump_file != NULL)
     fprintf (lra_dump_file,
             "     Frame pointer can not be eliminated anymore\n");
@@ -1422,9 +1430,10 @@
   CLEAR_HARD_REG_SET (set);
   add_to_hard_reg_set (&set, Pmode, HARD_FRAME_POINTER_REGNUM);
   n = spill_pseudos (set, spilled_pseudos);
-  for (ep = reg_eliminate; ep < &reg_eliminate[NUM_ELIMINABLE_REGS]; ep++)
-    if (ep->from == FRAME_POINTER_REGNUM && ep->to == STACK_POINTER_REGNUM)
-      setup_can_eliminate (ep, false);
+  if (!ep)
+    for (ep = reg_eliminate; ep < &reg_eliminate[NUM_ELIMINABLE_REGS]; ep++)
+      if (ep->from == FRAME_POINTER_REGNUM && ep->to == STACK_POINTER_REGNUM)
+       setup_can_eliminate (ep, false);
   return n;
 }
 

-- 
Alexandre Oliva, happy hacker            https://blog.lx.oliva.nom.br/
Free Software Activist     FSFLA co-founder     GNU Toolchain Engineer
More tolerance and less prejudice are key for inclusion and diversity.
Excluding neuro-others for not behaving ""normal"" is *not* inclusive!