| Message ID | a3fa64e0-bc2c-0c14-9062-d6f5c1690637@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [pushed,LRA] : Spill pseudos assigned to fp when fp->sp elimination became impossible | expand |
On Wed, 2023-08-16 at 12:13 -0400, Vladimir Makarov wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > The attached patch fixes recently found wrong insn removal in LRA port > for AVR. > > The patch was successfully tested and bootstrapped on x86-64 and aarch64. > > Hi Vladimir, Thanks for working on this. After applying the patch, I'm seeing that the pseudo in the frame pointer that got spilled is taking up the same stack slot that was already assigned to a spilled pseudo, and that is causing execution failure (it is also causing a crash when building libgcc for avr) (insn 108 15 3 3 (set (mem/c:SI (plus:HI (reg/f:HI 28 r28) (const_int 1 [0x1])) [3 %sfp+1 S4 A8]) (reg:SI 4 r4 [orig:46 f.3_4 ] [46])) "/home/i41766/code/personal/gcc/gcc/testsuite/gcc.c-torture/execute/20050224- 1.c":29:16 119 {*movsi_split} (nil)) (insn 3 108 4 3 (set (mem/c:HI (plus:HI (reg/f:HI 28 r28) (const_int 1 [0x1])) [3 %sfp+1 S2 A8]) (const_int 0 [0])) "/home/i41766/code/personal/gcc/gcc/testsuite/gcc.c-torture/execute/20050224-1.c":19:21 101 {*movhi_split} (expr_list:REG_EQUAL (const_int 0 [0]) (nil))) Both pseudo 46, and the pseudo spilled off FP (51) get assigned stack slot 0. This translates to this obviously wrong assembly code. lds r4,f lds r5,f+1 lds r6,f+2 lds r7,f+3 std Y+1,r4 std Y+2,r5 std Y+3,r6 std Y+4,r7 std Y+2,__zero_reg__ std Y+1,__zero_reg__ I tried a hacky workaround (see patch below) to create a new stack slot and assign the spilled pseudo to it, and that works. Not sure if that's the right way to do it though. Regards Senthil diff --git gcc/lra-spills.cc gcc/lra-spills.cc index 7e1d35b5e4e..e985ab56a60 100644 --- gcc/lra-spills.cc +++ gcc/lra-spills.cc @@ -359,11 +359,12 @@ add_pseudo_to_slot (int regno, int slot_num) length N. Sort pseudos in PSEUDO_REGNOS for subsequent assigning memory stack slots. */ static void -assign_stack_slot_num_and_sort_pseudos (int *pseudo_regnos, int n) +assign_stack_slot_num_and_sort_pseudos (int *pseudo_regnos, int n, bool reset) { int i, j, regno; - slots_num = 0; + if (reset) + slots_num = 0; /* Assign stack slot numbers to spilled pseudos, use smaller numbers for most frequently used pseudos. */ for (i = 0; i < n; i++) @@ -628,14 +629,15 @@ lra_spill (void) /* Sort regnos according their usage frequencies. */ qsort (pseudo_regnos, n, sizeof (int), regno_freq_compare); n = assign_spill_hard_regs (pseudo_regnos, n); - assign_stack_slot_num_and_sort_pseudos (pseudo_regnos, n); + assign_stack_slot_num_and_sort_pseudos (pseudo_regnos, n, true); for (i = 0; i < n; i++) if (pseudo_slots[pseudo_regnos[i]].mem == NULL_RTX) assign_mem_slot (pseudo_regnos[i]); - if ((n2 = lra_update_fp2sp_elimination (pseudo_regnos)) > 0) + if ((n2 = lra_update_fp2sp_elimination (&pseudo_regnos[n])) > 0) { + assign_stack_slot_num_and_sort_pseudos (&pseudo_regnos[n], n2, false); /* Assign stack slots to spilled pseudos assigned to fp. */ - for (i = 0; i < n2; i++) + for (i = n; i < n + n2; i++) if (pseudo_slots[pseudo_regnos[i]].mem == NULL_RTX) assign_mem_slot (pseudo_regnos[i]); } Regards Senthil
On 8/17/23 07:19, SenthilKumar.Selvaraj@microchip.com wrote: > On Wed, 2023-08-16 at 12:13 -0400, Vladimir Makarov wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> The attached patch fixes recently found wrong insn removal in LRA port >> for AVR. >> >> The patch was successfully tested and bootstrapped on x86-64 and aarch64. >> >> > Hi Vladimir, > > Thanks for working on this. After applying the patch, I'm seeing that the > pseudo in the frame pointer that got spilled is taking up the same stack > slot that was already assigned to a spilled pseudo, and that is causing execution > failure (it is also causing a crash when building libgcc for avr) > > ... > I tried a hacky workaround (see patch below) to create a new stack slot and > assign the spilled pseudo to it, and that works. > > Not sure if that's the right way to do it though. > The general way of solution is right but I've just committed a different version of the patch.
diff --git a/gcc/lra-eliminations.cc b/gcc/lra-eliminations.cc index 1f4e3fec9e0..3c58d4a3815 100644 --- a/gcc/lra-eliminations.cc +++ b/gcc/lra-eliminations.cc @@ -1086,18 +1086,18 @@ eliminate_regs_in_insn (rtx_insn *insn, bool replace_p, bool first_p, lra_update_insn_recog_data (insn); } -/* Spill pseudos which are assigned to hard registers in SET. Add - affected insns for processing in the subsequent constraint - pass. */ -static void -spill_pseudos (HARD_REG_SET set) +/* Spill pseudos which are assigned to hard registers in SET, record them in + SPILLED_PSEUDOS unless it is null, and return the recorded pseudos number. + Add affected insns for processing in the subsequent constraint pass. */ +static int +spill_pseudos (HARD_REG_SET set, int *spilled_pseudos) { - int i; + int i, n; bitmap_head to_process; rtx_insn *insn; if (hard_reg_set_empty_p (set)) - return; + return 0; if (lra_dump_file != NULL) { fprintf (lra_dump_file, " Spilling non-eliminable hard regs:"); @@ -1107,6 +1107,7 @@ spill_pseudos (HARD_REG_SET set) fprintf (lra_dump_file, "\n"); } bitmap_initialize (&to_process, ®_obstack); + n = 0; for (i = FIRST_PSEUDO_REGISTER; i < max_reg_num (); i++) if (lra_reg_info[i].nrefs != 0 && reg_renumber[i] >= 0 && overlaps_hard_reg_set_p (set, @@ -1116,6 +1117,8 @@ spill_pseudos (HARD_REG_SET set) fprintf (lra_dump_file, " Spilling r%d(%d)\n", i, reg_renumber[i]); reg_renumber[i] = -1; + if (spilled_pseudos != NULL) + spilled_pseudos[n++] = i; bitmap_ior_into (&to_process, &lra_reg_info[i].insn_bitmap); } lra_no_alloc_regs |= set; @@ -1126,6 +1129,7 @@ spill_pseudos (HARD_REG_SET set) lra_set_used_insn_alternative (insn, LRA_UNKNOWN_ALT); } bitmap_clear (&to_process); + return n; } /* Update all offsets and possibility for elimination on eliminable @@ -1238,7 +1242,7 @@ update_reg_eliminate (bitmap insns_with_changed_offsets) } lra_no_alloc_regs |= temp_hard_reg_set; eliminable_regset &= ~temp_hard_reg_set; - spill_pseudos (temp_hard_reg_set); + spill_pseudos (temp_hard_reg_set, NULL); return result; } @@ -1382,15 +1386,17 @@ process_insn_for_elimination (rtx_insn *insn, bool final_p, bool first_p) /* Update frame pointer to stack pointer elimination if we started with permitted frame pointer elimination and now target reports that we can not - do this elimination anymore. */ -void -lra_update_fp2sp_elimination (void) + do this elimination anymore. Record spilled pseudos in SPILLED_PSEUDOS + unless it is null, and return the recorded pseudos number. */ +int +lra_update_fp2sp_elimination (int *spilled_pseudos) { + int n; HARD_REG_SET set; class lra_elim_table *ep; if (frame_pointer_needed || !targetm.frame_pointer_required ()) - return; + return 0; gcc_assert (!elimination_fp2sp_occured_p); if (lra_dump_file != NULL) fprintf (lra_dump_file, @@ -1398,10 +1404,11 @@ lra_update_fp2sp_elimination (void) frame_pointer_needed = true; CLEAR_HARD_REG_SET (set); add_to_hard_reg_set (&set, Pmode, HARD_FRAME_POINTER_REGNUM); - spill_pseudos (set); + n = spill_pseudos (set, spilled_pseudos); for (ep = reg_eliminate; ep < ®_eliminate[NUM_ELIMINABLE_REGS]; ep++) if (ep->from == FRAME_POINTER_REGNUM && ep->to == STACK_POINTER_REGNUM) setup_can_eliminate (ep, false); + return n; } /* Entry function to do final elimination if FINAL_P or to update diff --git a/gcc/lra-int.h b/gcc/lra-int.h index 633d9af8058..d0752c2ae50 100644 --- a/gcc/lra-int.h +++ b/gcc/lra-int.h @@ -414,7 +414,7 @@ extern int lra_get_elimination_hard_regno (int); extern rtx lra_eliminate_regs_1 (rtx_insn *, rtx, machine_mode, bool, bool, poly_int64, bool); extern void eliminate_regs_in_insn (rtx_insn *insn, bool, bool, poly_int64); -extern void lra_update_fp2sp_elimination (void); +extern int lra_update_fp2sp_elimination (int *spilled_pseudos); extern void lra_eliminate (bool, bool); extern poly_int64 lra_update_sp_offset (rtx, poly_int64); diff --git a/gcc/lra-spills.cc b/gcc/lra-spills.cc index fe58f162d05..7e1d35b5e4e 100644 --- a/gcc/lra-spills.cc +++ b/gcc/lra-spills.cc @@ -606,7 +606,7 @@ lra_need_for_spills_p (void) void lra_spill (void) { - int i, n, curr_regno; + int i, n, n2, curr_regno; int *pseudo_regnos; regs_num = max_reg_num (); @@ -632,8 +632,14 @@ lra_spill (void) for (i = 0; i < n; i++) if (pseudo_slots[pseudo_regnos[i]].mem == NULL_RTX) assign_mem_slot (pseudo_regnos[i]); - lra_update_fp2sp_elimination (); - if (n > 0 && crtl->stack_alignment_needed) + if ((n2 = lra_update_fp2sp_elimination (pseudo_regnos)) > 0) + { + /* Assign stack slots to spilled pseudos assigned to fp. */ + for (i = 0; i < n2; i++) + if (pseudo_slots[pseudo_regnos[i]].mem == NULL_RTX) + assign_mem_slot (pseudo_regnos[i]); + } + if (n + n2 > 0 && crtl->stack_alignment_needed) /* If we have a stack frame, we must align it now. The stack size may be a part of the offset computation for register elimination. */
The attached patch fixes recently found wrong insn removal in LRA port for AVR. The patch was successfully tested and bootstrapped on x86-64 and aarch64. commit 748a77558ff37761faa234e19327ad1decaace33 Author: Vladimir N. Makarov <vmakarov@redhat.com> Date: Wed Aug 16 09:13:54 2023 -0400 [LRA]: Spill pseudos assigned to fp when fp->sp elimination became impossible Porting LRA to AVR revealed that creating a stack slot can make fp->sp elimination impossible. The previous patches undoes fp assignment after the stack slot creation but calculated wrongly live info after this. This resulted in wrong generation by deleting some still alive insns. This patch fixes this problem. gcc/ChangeLog: * lra-int.h (lra_update_fp2sp_elimination): Change the prototype. * lra-eliminations.cc (spill_pseudos): Record spilled pseudos. (lra_update_fp2sp_elimination): Ditto. (update_reg_eliminate): Adjust spill_pseudos call. * lra-spills.cc (lra_spill): Assign stack slots to pseudos spilled in lra_update_fp2sp_elimination.