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
