Christophe, thank you very much for the detailed feedback. I re-posted the patches with your suggested changes to ChangeLog formatting, added line breaks for the long lines, and updated the test cases so you can now run them without the --target-board option. Regarding auto-merging, my changes are in a local branch; I did "git rebase master" and it said my branch was up to date but it did not change the original base hash of the file (I don't think arm.cc has changed much recently anyway) so I'm not sure why it wasn't able to merge.
Re-posted patches: https://gcc.gnu.org/pipermail/gcc-patches/2025-July/689141.html https://gcc.gnu.org/pipermail/gcc-patches/2025-July/689142.html Sincerely, Matt > On 07/09/2025 12:04 PM EDT Christophe Lyon <christophe.l...@linaro.org> wrote: > > > Hi, > > Thanks for the patch, the code change looks sensible to me, but you'll > have to wait for Richard (in cc) as he is the maintainer. > However I have a few comments below: > > On Thu, 26 Jun 2025 at 00:57, Matt Parks <matt.pa...@go-aps.com> wrote: > > > > Trying again, hopefully formatted correctly this time, and now including a > > test case. Test case fails with original code, passes with patch. Command > > to execute test case: > > make check-c RUNTESTFLAGS="--target-board='arm-sim/-march=armv5t' > > arm.exp=pr117366.c" > > > The commit message should include a description of the fix (probably > along the lines of what you wrote in bugzilla). > > For better test coverage, I think such a --tarbet-board option should > not be needed (see below) > > > gcc/ChangeLog: > > * arm.cc: fix thumb1 size-optimized function prolog violates > > -ffixed-rX > > Sorry for the nitpicking, but this is not the expected format; you > should include the name of the function you modify, and the sentence > should end with a full stop. > Something like this: > * arm.cc (thumb1_extra_regs_pushed): Take fixed regs into account. > > > > > diff --git a/gcc/config/arm/arm.cc b/gcc/config/arm/arm.cc > > index bde06f3fa86..65a161a64ff 100644 > > --- a/gcc/config/arm/arm.cc > > +++ b/gcc/config/arm/arm.cc > > @@ -26746,8 +26746,8 @@ thumb1_extra_regs_pushed (arm_stack_offsets > > *offsets, bool for_prologue) > > live_regs_mask >>= reg_base; > > } > > - while (reg_base + n_free < 8 && !(live_regs_mask & 1) > > - && (for_prologue || call_used_or_fixed_reg_p (reg_base + n_free))) > > + while (reg_base + n_free <= LAST_LO_REGNUM && !(live_regs_mask & 1) > > + && (for_prologue || (call_used_or_fixed_reg_p (reg_base + n_free) > > && !fixed_regs[reg_base + n_free]))) > The line is now becoming too log, please split it (and indent accordingly) > > > { > > live_regs_mask >>= 1; > > n_free++; > > diff --git a/gcc/testsuite/gcc.target/arm/pr117366.c > > b/gcc/testsuite/gcc.target/arm/pr117366.c > > new file mode 100644 > > index 00000000000..89ecd950b7a > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/arm/pr117366.c > > @@ -0,0 +1,14 @@ > > + > > +/* { dg-do compile } */ > > +/* { dg-options "-Os -ffixed-r4 -ffixed-r5 -ffixed-r6 -ffixed-r7" } */ > > +/* { dg-require-effective-target arm_thumb1_ok } */ > This checks if the current target generates thumb1 (hence you need the > --target-board option). > I think you could instead use arm_arch_v5t_thumb_ok... > > > +/* { dg-additional-options "-march=armv5t -mthumb" {target > > arm_arch_v5t_ok} } */ > ... and use /* { dg-add-options arm_arch_v5t_thumb } */ > > > +void func(void *, void *, void *, int, int); > > + > > +int bad_func(void) { > > + int a, b, c; > > + func(&a, &b, &c, 1, 2); > > + return b; > > +} > > + > > +/* { dg-final { scan-assembler-not "pop.*r\[4567\]" } } */ > > > > Finally, for some reason the automated CI was not able to apply your > patch on top of "recent" trunk, can you rebase your patch? > > Thanks > > Christophe
