On Jun 25, 2025, Vladimir Makarov <vmaka...@redhat.com> wrote:
> Alex, thanks for investigation of corner cases of register elimination.
You're welcome. Thanks for the reviews
> I guess it is too strict.
Yeah. I have a less strict version that relaxes it enough to not
regress acats-4 on arm-linux-gnueabihf, and to complete bootstrap on
x86_64-linux-gnu with ix86_frame_pointer_required modified to return
true on nonzero frame sizes, adding -maccumulate-outgoing-args to
BOOT_CFLAGS and TFLAGS.
I pushed the patchset with it to users/aoliva/heads/lra-elim-fp2sp
> I am not ready to answer the question about committing the patch right
> now.
ACK. This one isn't critical (as in, it doesn't fix any wrong code, it
only catches and prevents some known cases of silent wrong code).
However, the other patches that I posted recently, along with this one,
fix actual problems that cause significant disruption for GNAT on
arm-linux-gnueabihf.
One of them fixes a regression on arm-linux-gnueabihf that would be
introduced by another you've already reviewed but I haven't installed
yet (because of the regression); but refraining from installing it would
leave an avr regression unfixed.
Hopefully you only meant that this one patch needed more time, while the
others, that are simpler fixes, could be reviewed and and go in.
Otherwise, maybe the way to go is to revert the PR120424 change that
went in, and install a workaround for arm that sacrifices a frame
pointer optimization to stop it from tripping the lra elimination issues
that I've identified and fixed.
> In general I think we should have functionality generating the right
> code whenever any elimination goes prohibited or enabled back and
> forth during LRA work. That is what I would like to aim at. It might
> require considerable review of all existing elimination code.
*nod*, we're not there yet, and I wasn't really aiming that high. I
only set out to avoid silent wrong code with the general strategy that's
already implemented, in which eliminations remain prohibited once
they're prohibition is noticed. I have hunches about how to allow that,
even with nonzero sp offsets, but I have run out of time to implement
them myself.
> So I think the best way right now is to fill a PR.
I nearly did, but then I realized 120424 already grouped all of these
patches, so I unduped it and linked to all related patches from it.
[lra] catch all to-sp eliminations with nonzero offsets [PR120424]
An x86_64-linux-gnu native with ix86_frame_pointer_required modified
to return true for nonzero frames, to exercize
lra_update_fp2sp_elimination, reveals in stage1 testing that wrong
code is generated for gcc.c-torture/execute/ieee/fp-cmp-8l.c:
argp-to-sp eliminations are used for one_test to pass its arguments on
to *pos, and the sp offsets survive the disabling of that elimination.
We didn't really have to disable that elimination, but the x86 backend
disables eliminations to sp if frame_pointer_needed.
This change extends the catching of fp2sp eliminations to all (?)
eliminations to sp with nonzero offsets, since none of them can be
properly reversed and would silently lead to wrong code.
By accepting nonzero offsets, we bootstrap with
-maccumulate-outgoing-args on x86_64-linux-gnu (with
ix86_frame_pointer_required modified to return true on nonzero frame
size).
for gcc/ChangeLog
PR rtl-optimization/120424
* lra-eliminations.cc (elimination_2sp_occurred_p): Rename
from...
(elimination_fp2sp_occured_p): ... this. Adjust all uses.
(lra_eliminate_regs_1): Don't require a from-frame-pointer
elimination to set it.
(update_reg_eliminate): Likewise to test it.
---
gcc/lra-eliminations.cc | 46 +++++++++++++++++++++++++---------------------
1 file changed, 25 insertions(+), 21 deletions(-)
diff --git a/gcc/lra-eliminations.cc b/gcc/lra-eliminations.cc
index 9cdd0c5ff53a2..045f2dcf23ef7 100644
--- a/gcc/lra-eliminations.cc
+++ b/gcc/lra-eliminations.cc
@@ -309,8 +309,18 @@ move_plus_up (rtx x)
return x;
}
-/* Flag that we already did frame pointer to stack pointer elimination. */
-static bool elimination_fp2sp_occured_p = false;
+/* Flag that we already applied nonzero stack pointer elimination
+ offset; such sp updates cannot currently be undone. */
+static bool elimination_2sp_occurred_p = false;
+
+/* Take note of any nonzero sp-OFFSET used in eliminations to sp. */
+static inline poly_int64
+note_spoff (poly_int64 offset)
+{
+ if (maybe_ne (offset, 0))
+ elimination_2sp_occurred_p = true;
+ return offset;
+}
/* Scan X and replace any eliminable registers (such as fp) with a
replacement (such as sp) if SUBST_P, plus an offset. The offset is
@@ -369,13 +379,10 @@ lra_eliminate_regs_1 (rtx_insn *insn, rtx x, machine_mode
mem_mode,
{
rtx to = subst_p ? ep->to_rtx : ep->from_rtx;
- if (ep->to_rtx == stack_pointer_rtx && ep->from ==
FRAME_POINTER_REGNUM)
- elimination_fp2sp_occured_p = true;
-
if (maybe_ne (update_sp_offset, 0))
{
if (ep->to_rtx == stack_pointer_rtx)
- return plus_constant (Pmode, to, update_sp_offset);
+ return plus_constant (Pmode, to, note_spoff (update_sp_offset));
return to;
}
else if (update_p)
@@ -385,7 +392,8 @@ lra_eliminate_regs_1 (rtx_insn *insn, rtx x, machine_mode
mem_mode,
ep->offset
- (insn != NULL_RTX
&& ep->to_rtx == stack_pointer_rtx
- ? lra_get_insn_recog_data (insn)->sp_offset
+ ? note_spoff (lra_get_insn_recog_data
+ (insn)->sp_offset)
: 0));
else
return to;
@@ -402,19 +410,18 @@ lra_eliminate_regs_1 (rtx_insn *insn, rtx x, machine_mode
mem_mode,
poly_int64 offset, curr_offset;
rtx to = subst_p ? ep->to_rtx : ep->from_rtx;
- if (ep->to_rtx == stack_pointer_rtx && ep->from ==
FRAME_POINTER_REGNUM)
- elimination_fp2sp_occured_p = true;
-
if (! update_p && ! full_p)
return simplify_gen_binary (PLUS, Pmode, to, XEXP (x, 1));
if (maybe_ne (update_sp_offset, 0))
- offset = ep->to_rtx == stack_pointer_rtx ? update_sp_offset : 0;
+ offset = (ep->to_rtx == stack_pointer_rtx
+ ? note_spoff (update_sp_offset)
+ : 0);
else
offset = (update_p
? ep->offset - ep->previous_offset : ep->offset);
if (full_p && insn != NULL_RTX && ep->to_rtx == stack_pointer_rtx)
- offset -= lra_get_insn_recog_data (insn)->sp_offset;
+ offset -= note_spoff (lra_get_insn_recog_data
(insn)->sp_offset);
if (poly_int_rtx_p (XEXP (x, 1), &curr_offset)
&& known_eq (curr_offset, -offset))
return to;
@@ -465,15 +472,13 @@ lra_eliminate_regs_1 (rtx_insn *insn, rtx x, machine_mode
mem_mode,
{
rtx to = subst_p ? ep->to_rtx : ep->from_rtx;
- if (ep->to_rtx == stack_pointer_rtx && ep->from ==
FRAME_POINTER_REGNUM)
- elimination_fp2sp_occured_p = true;
-
if (maybe_ne (update_sp_offset, 0))
{
if (ep->to_rtx == stack_pointer_rtx)
return plus_constant (Pmode,
gen_rtx_MULT (Pmode, to, XEXP (x, 1)),
- update_sp_offset * INTVAL (XEXP (x, 1)));
+ note_spoff (update_sp_offset)
+ * INTVAL (XEXP (x, 1)));
return gen_rtx_MULT (Pmode, to, XEXP (x, 1));
}
else if (update_p)
@@ -486,7 +491,7 @@ lra_eliminate_regs_1 (rtx_insn *insn, rtx x, machine_mode
mem_mode,
poly_int64 offset = ep->offset;
if (insn != NULL_RTX && ep->to_rtx == stack_pointer_rtx)
- offset -= lra_get_insn_recog_data (insn)->sp_offset;
+ offset -= note_spoff (lra_get_insn_recog_data
(insn)->sp_offset);
return
plus_constant (Pmode,
gen_rtx_MULT (Pmode, to, XEXP (x, 1)),
@@ -1213,8 +1218,7 @@ update_reg_eliminate (bitmap insns_with_changed_offsets)
pointer elimination the condition is a bit relaxed and we just
require
that actual elimination has not been done yet. */
gcc_assert (ep->to_rtx != stack_pointer_rtx
- || (ep->from == FRAME_POINTER_REGNUM
- && !elimination_fp2sp_occured_p)
+ || !elimination_2sp_occurred_p
|| (ep->from < FIRST_PSEUDO_REGISTER
&& fixed_regs [ep->from]));
@@ -1429,7 +1433,7 @@ lra_update_fp2sp_elimination (int *spilled_pseudos)
if (frame_pointer_needed || !targetm.frame_pointer_required ())
return 0;
- gcc_assert (!elimination_fp2sp_occured_p);
+ gcc_assert (!elimination_2sp_occurred_p);
ep = elimination_map[FRAME_POINTER_REGNUM];
if (ep->to == STACK_POINTER_REGNUM)
{
@@ -1499,7 +1503,7 @@ lra_eliminate (bool final_p, bool first_p)
if (first_p)
{
- elimination_fp2sp_occured_p = false;
+ elimination_2sp_occurred_p = false;
init_elimination ();
}
[TESTING] [lra] [x86] require frame pointer on nonzero frame size [PR120424]
FTR, this is what I used to test the above.
diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
index 84081ab126705..31e0f8a453de9 100644
--- a/gcc/config/i386/i386.cc
+++ b/gcc/config/i386/i386.cc
@@ -6035,6 +6035,11 @@ ix86_frame_pointer_required (void)
if (crtl->profile && !flag_fentry)
return true;
+ /* Exercise lra_update_fp2sp_elimination.
+ Requires -maccumulate-outgoing-args. */
+ if (ix86_get_frame_size ())
+ return true;
+
return false;
}
--
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!
