Hi, this is Ping #2 for the patch below.
https://sourceware.org/pipermail/binutils/2025-February/139497.html
https://sourceware.org/PR32704
Ok to apply to trunk? (I have write-after-approval).
Johann
--
gas/
PR gas/32704
PR gas/21683
* config/tc-avr.c (avr_isr): bool-ize.
(avr_emit_insn): Emit "mov" code as MOV R1,<reg>.
(avr_isr_stack_t): New typedef.
(avr_emit_push, avr_emit_pop): New static functions.
(avr_patch_gccisr_frag): Overhaul prologue and epilogue
generation.
--
> The prologue generated by __gcc_isr can be improved in
> situations where:
>
> * ZERO_REG is needed, and
> * SREG is not clobbered by the ISR, and
> * avr-gcc provides a GPR >= R16 with the Done chunk, and
> * Code generation is for ordinary AVRs (not AVRrc).
>
> For example, the prologue for
>
> volatile char var;
>
> __attribute__((signal)) void __vector_1 (void)
> {
> var = 1;
> var = 0;
> }
>
> may be
>
> 00000000 <__vector_1>:
> 0: 8f 93 push r24
> 2: 1f 92 push r1
> 4: 80 e0 ldi r24, 0
> 6: 18 2e mov r1, r24
>
> instead of the code as currently generated by GAS:
>
> 00000000 <__vector_1>:
> 0: 1f 92 push r1
> 2: 1f b6 in r1, SREG
> 4: 1f 92 push r1
> 6: 11 24 clr r1
> 8: 8f 93 push r24
>
> which consumes more stack, time and code than needed.
>
> The patch is for trunk. [...]
>
> Johann
>
> --
>
> ChangeLog:
>
> AVR: gas/32704 - Improve code generation for __gcc_isr.
>
> The prologue generated by __gcc_isr can be improved in
> situations where:
>
> * ZERO_REG is needed, and
> * SREG is not clobbered by the ISR, and
> * avr-gcc provides a GPR >= R16 with the Done chunk, and
> * Code generation is for ordinary AVRs (not AVRrc).
>
> For example, the prologue for
>
> volatile char var;
>
> __attribute__((signal)) void __vector_1 (void)
> {
> var = 1;
> var = 0;
> }
>
> may be
>
> 00000000 <__vector_1>:
> 0: 8f 93 push r24
> 2: 1f 92 push r1
> 4: 80 e0 ldi r24, 0
> 6: 18 2e mov r1, r24
>
> instead of the code as currently generated by GAS:
>
> 00000000 <__vector_1>:
> 0: 1f 92 push r1
> 2: 1f b6 in r1, SREG
> 4: 1f 92 push r1
> 6: 11 24 clr r1
> 8: 8f 93 push r24
>
> which consumes more stack, time and code than needed.
>
> gas/
> PR gas/32704
> PR gas/21683
> * config/tc-avr.c (avr_isr): bool-ize.
> (avr_emit_insn): Emit "mov" code as MOV R1,<reg>.
> (avr_isr_stack_t): New typedef.
> (avr_emit_push, avr_emit_pop): New static functions.
> (avr_patch_gccisr_frag): Overhaul prologue and epilogue
> generation.
> -------------- next part --------------
> AVR: gas/32704 - Improve code generation for __gcc_isr.
>
> The prologue generated by __gcc_isr can be improved in
> situations where:
>
> * ZERO_REG is needed, and
> * SREG is not clobbered by the ISR, and
> * avr-gcc provides a GPR >= R16 with the Done chunk, and
> * Code generation is for ordinary AVRs (not AVRrc).
>
> For example, the prologue for
>
> volatile char var;
>
> __attribute__((signal)) void __vector_1 (void)
> {
> var = 1;
> var = 0;
> }
>
> may be
>
> 00000000 <__vector_1>:
> 0: 8f 93 push r24
> 2: 1f 92 push r1
> 4: 80 e0 ldi r24, 0
> 6: 18 2e mov r1, r24
>
> instead of the code as currently generated by GAS:
>
> 00000000 <__vector_1>:
> 0: 1f 92 push r1
> 2: 1f b6 in r1, SREG
> 4: 1f 92 push r1
> 6: 11 24 clr r1
> 8: 8f 93 push r24
>
> which consumes more stack, time and code than needed.
>
> gas/
> PR gas/32704
> PR gas/21683
> * config/tc-avr.c (avr_isr): bool-ize.
> (avr_emit_insn): Emit "mov" code as MOV R1,<reg>.
> (avr_isr_stack_t): New typedef.
> (avr_emit_push, avr_emit_pop): New static functions.
> (avr_patch_gccisr_frag): Overhaul prologue and epilogue
> generation.
@@ -146,9 +146,9 @@ static struct
/* Set and used during parse from chunk 1 (Prologue) up to chunk 0 (Done).
Set by `avr_update_gccisr' and used by `avr_patch_gccisr_frag'. */
- int need_reg_tmp;
- int need_reg_zero;
- int need_sreg;
+ bool need_reg_tmp;
+ bool need_reg_zero;
+ bool need_sreg;
} avr_isr;
static void avr_gccisr_operands (struct avr_opcodes_s*, char**);
@@ -2500,7 +2500,7 @@ avr_update_gccisr (struct avr_opcodes_s *opcode, int reg1, int reg2)
of octets written. INSN specifies the desired instruction and REG is the
register used by it. This function is only used with restricted subset of
instructions as might be emit by `__gcc_isr'. IN / OUT will use SREG
- and LDI loads 0. */
+ and LDI loads 0. MOV sets R1. */
static void
avr_emit_insn (const char *insn, int reg, char **pwhere)
@@ -2510,13 +2510,21 @@ avr_emit_insn (const char *insn, int reg, char **pwhere)
const struct avr_opcodes_s *op
= (struct avr_opcodes_s*) str_hash_find (avr_hash, insn);
- /* We only have to deal with: IN, OUT, PUSH, POP, CLR, LDI 0. All of
- these deal with at least one Reg and are 1-word instructions. */
+ /* We only have to deal with: IN, OUT, PUSH, POP, CLR, LDI 0, MOV R1.
+ All of these deal with at least one Reg and are 1-word instructions. */
gas_assert (op && 1 == op->insn_size);
gas_assert (reg >= 0 && reg <= 31);
- if (strchr (op->constraints, 'r'))
+ if (!strcmp (insn, "mov"))
+ {
+ const int reg1 = 1;
+ /* AVR_INSN (mov, "r,r", "001011rdddddrrrr", ...) */
+ bin = op->bin_opcode | (reg1 << 4);
+ bin |= reg & 0xf;
+ bin |= (reg & 0x10) << 5;
+ }
+ else if (strchr (op->constraints, 'r'))
{
bin = op->bin_opcode | (reg << 4);
}
@@ -2538,12 +2546,68 @@ avr_emit_insn (const char *insn, int reg, char **pwhere)
}
else
gas_assert (0 == strcmp ("r", op->constraints)
+ || 0 == strcmp ("mov", op->name)
|| 0 == strcmp ("ldi", op->name));
bfd_putl16 ((bfd_vma) bin, *pwhere);
(*pwhere) += 2 * op->insn_size;
}
+typedef struct
+{
+ unsigned reg_mask;
+ int n_pushed;
+ int slot[5];
+} avr_isr_stack_t;
+
+/* Encode / decode that the register operation is on behalf of SREG. */
+#define FOR_SREG(x) (-(x) - 1)
+
+
+static void
+avr_emit_push (avr_isr_stack_t *st, int r, char **pwhere, bool emit_code_p)
+{
+ const bool for_sreg = r < 0;
+ const int reg = for_sreg ? FOR_SREG (r) : r;
+
+ /* When REG has already been pushed, don't push it again
+ (except it is for SREG). */
+
+ if (!for_sreg && (st->reg_mask & (1u << reg)))
+ return;
+
+ st->slot[st->n_pushed] = r;
+ st->n_pushed += 1;
+ st->reg_mask |= 1u << reg;
+
+ if (emit_code_p)
+ {
+ if (for_sreg)
+ {
+ avr_emit_insn ("in", reg, pwhere);
+ avr_emit_insn ("push", reg, pwhere);
+ }
+ else
+ avr_emit_insn ("push", reg, pwhere);
+ }
+}
+
+
+static void
+avr_emit_pop (int r, char **pwhere)
+{
+ const bool for_sreg = r < 0;
+ const int reg = for_sreg ? FOR_SREG (r) : r;
+
+ if (for_sreg)
+ {
+ avr_emit_insn ("pop", reg, pwhere);
+ avr_emit_insn ("out", reg, pwhere);
+ }
+ else
+ avr_emit_insn ("pop", reg, pwhere);
+}
+
/* Turn rs_machine_dependent frag *FR into an ordinary rs_fill code frag,
using information gathered in `avr_isr'. REG is the register number as
@@ -2553,15 +2617,29 @@ static void
avr_patch_gccisr_frag (fragS *fr, int reg)
{
int treg;
- int n_pushed = 0;
char *where = fr->fr_literal;
- const int tiny_p = avr_mcu->mach == bfd_mach_avrtiny;
+ const bool in_prologue = fr->fr_subtype == ISR_CHUNK_Prologue;
+ const bool in_epilogue = fr->fr_subtype == ISR_CHUNK_Epilogue;
+ const bool tiny_p = avr_mcu->mach == bfd_mach_avrtiny;
const int reg_tmp = tiny_p ? 16 : 0;
const int reg_zero = 1 + reg_tmp;
+ /* Whether to use MOV R1,* to set zero_reg on non-Tiny. */
+ bool mov_r1_p = false;
+ avr_isr_stack_t st = { 0, 0, { 0 } };
+
+ gas_assert (in_prologue ^ in_epilogue);
- /* Clearing ZERO_REG on non-Tiny needs CLR which clobbers SREG. */
+ /* Clearing ZERO_REG on non-Tiny needs CLR which clobbers SREG. Hence
+ when ZERO_REG is needed but SREG is not already clobbererd, and we have
+ a d-reg to play with, then use LDI + MOV to set ZERO_REG (PR32704). */
- avr_isr.need_sreg |= !tiny_p && avr_isr.need_reg_zero;
+ if (!tiny_p && avr_isr.need_reg_zero && !avr_isr.need_sreg)
+ {
+ if (reg >= 16)
+ mov_r1_p = true;
+ else
+ avr_isr.need_sreg = true;
+ }
/* A working register to PUSH / POP the SREG. We might use the register
as supplied by ISR_CHUNK_Done for that purpose as GCC wants to push
@@ -2569,7 +2647,8 @@ avr_patch_gccisr_frag (fragS *fr, int reg)
no additional regs to safe) and we use that reg. */
treg
- = avr_isr.need_reg_tmp ? reg_tmp
+ = mov_r1_p ? reg
+ : avr_isr.need_reg_tmp ? reg_tmp
: avr_isr.need_reg_zero ? reg_zero
: avr_isr.need_sreg ? reg
: reg > reg_zero ? reg
@@ -2577,68 +2656,57 @@ avr_patch_gccisr_frag (fragS *fr, int reg)
if (treg >= 0)
{
- /* Non-empty prologue / epilogue */
+ /* Non-empty prologue / epilogue.
- if (ISR_CHUNK_Prologue == fr->fr_subtype)
- {
- avr_emit_insn ("push", treg, &where);
- n_pushed++;
+ This code runs for prologue AND epilogue but only outputs insns
+ when in prologue. When in epilogue, still record which pushes
+ have been performed. */
- if (avr_isr.need_sreg)
- {
- avr_emit_insn ("in", treg, &where);
- avr_emit_insn ("push", treg, &where);
- n_pushed++;
- }
+ avr_emit_push (&st, treg, &where, in_prologue);
- if (avr_isr.need_reg_zero)
+ if (avr_isr.need_sreg)
+ avr_emit_push (&st, FOR_SREG (treg), &where, in_prologue);
+
+ if (avr_isr.need_reg_zero)
+ {
+ avr_emit_push (&st, reg_zero, &where, in_prologue);
+
+ if (in_prologue)
{
- if (reg_zero != treg)
+ if (mov_r1_p)
{
- avr_emit_insn ("push", reg_zero, &where);
- n_pushed++;
+ avr_emit_insn ("ldi", treg, &where);
+ avr_emit_insn ("mov", treg, &where);
}
- avr_emit_insn (tiny_p ? "ldi" : "clr", reg_zero, &where);
- }
-
- if (reg > reg_zero && reg != treg)
- {
- avr_emit_insn ("push", reg, &where);
- n_pushed++;
+ else
+ avr_emit_insn (tiny_p ? "ldi" : "clr", reg_zero, &where);
}
}
- else if (ISR_CHUNK_Epilogue == fr->fr_subtype)
- {
- /* Same logic as in Prologue but in reverse order and with counter
- parts of either instruction: POP instead of PUSH and OUT instead
- of IN. Clearing ZERO_REG has no couter part. */
- if (reg > reg_zero && reg != treg)
- avr_emit_insn ("pop", reg, &where);
+ if (avr_isr.need_reg_tmp)
+ avr_emit_push (&st, reg_tmp, &where, in_prologue);
- if (avr_isr.need_reg_zero
- && reg_zero != treg)
- avr_emit_insn ("pop", reg_zero, &where);
+ if (reg > reg_zero)
+ avr_emit_push (&st, reg, &where, in_prologue);
- if (avr_isr.need_sreg)
- {
- avr_emit_insn ("pop", treg, &where);
- avr_emit_insn ("out", treg, &where);
- }
+ /* Same logic like in Prologue but in reverse order and with counter-
+ parts of either instruction: POP instead of PUSH and OUT instead
+ of IN. Clearing ZERO_REG has no counterpart. */
- avr_emit_insn ("pop", treg, &where);
+ if (in_epilogue)
+ {
+ for (int i = st.n_pushed - 1; i >= 0; --i)
+ avr_emit_pop (st.slot[i], &where);
}
- else
- abort();
} /* treg >= 0 */
- if (ISR_CHUNK_Prologue == fr->fr_subtype
+ if (in_prologue
&& avr_isr.sym_n_pushed)
{
symbolS *sy = avr_isr.sym_n_pushed;
/* Turn magic `__gcc_isr.n_pushed' into its now known value. */
- S_SET_VALUE (sy, n_pushed);
+ S_SET_VALUE (sy, st.n_pushed);
S_SET_SEGMENT (sy, expr_section);
avr_isr.sym_n_pushed = NULL;
}