On Thu, 21 Nov 2024, Richard Sandiford wrote:
> > }
> >
> > +void
> > +aarch64_print_reg (rtx x, int code, FILE *file)
> > +{
> > + aarch64_print_operand (file, x, code);
> > +}
>
> Missing comment above function (to describe the parameters). But since
> aarch64_print_operand's parameter order is the standard one (for
> TARGET_PRINT_OPERAND), could we instead adjust the callers of
> aarch64_print_reg to use that order too, perhaps via
>
> targetm.asm_out.print_operand
>
> ?
aarch64_print_reg is not needed anymore after refactoring.
> > flag_mrecip_low_precision_sqrt = true;
> > +
> > + /* Enable unwind tables for MS. */
> > +#if defined (TARGET_AARCH64_MS_ABI)
> > + if (opts->x_flag_unwind_tables == 0)
> > + opts->x_flag_unwind_tables = 1;
> > +#endif // TARGET_AARCH64_MS_ABI
>
> The usual way to do this is via:
>
> SET_OPTION_IF_UNSET (opts, opts_set, flag_unwind_tables, 1);
>
> (which is also what x86 seems to use). This means that an explicit
> -fno-unwind-tables still works.
>
> Would that work here too, or were you deliberately trying to override
> -fno-unwind-tables?
>
Thanks for the clarification. SET_OPTION_IF_UNSET will be applied.
> > +#define TARGET_ASM_FUNCTION_END_PROLOGUE mingw_pe_seh_end_prologue
> > +#undef TARGET_ASM_EMIT_EXCEPT_PERSONALITY
> > +#define TARGET_ASM_EMIT_EXCEPT_PERSONALITY
> > mingw_pe_seh_emit_except_personality
> > +#undef TARGET_ASM_INIT_SECTIONS
> > +#define TARGET_ASM_INIT_SECTIONS mingw_pe_seh_init_sections
> > +#undef SUBTARGET_ASM_UNWIND_INIT
> > +#define SUBTARGET_ASM_UNWIND_INIT mingw_pe_seh_init
>
> Could we move more of this into a common header, shared with x86?
> Many of the definitions seem to be largely the same.
It would require extra refactoring and careful validation for ix86 changes.
It deserves a separate patch. Once aarch64/cygming.h is close enough to
i386/cygming.h, it will be moved to config/mingw.
> > + || ((r) >= V8_REGNUM && (r) <= V15_REGNUM))
> > +#else
> > +#define CALLEE_SAVED_REG_NUMBER(r) 0
> > +#endif
> > +
> > +static HOST_WIDE_INT
> > +seh_parallel_offset (rtx pat, HOST_WIDE_INT wanted_regnum)
>
> There should be a comment above the function describing what it does
> and what the parameters are. Same for the others in the file.
The comments will be added above new functions for SEH.
> > + src = SET_SRC (ele);
> > +
> > + if (GET_CODE (dest) == REG
> > + && GET_CODE (src) == PLUS
> > + && XEXP (src, 0) == stack_pointer_rtx)
> > + {
> > + increment = INTVAL (XEXP (src, 1));
>
> We can't assume without checking that XEXP (src, 1) is a CONST_INT.
> E.g. it might be a CONST_POLY_INT for functions with SVE spills.
> It's OK to punt on that for now, but I think it at least needs
> a sorry() diagnostic. As things stand, we'd ICE for development
> builds and generate silent wrong code for release builds.
>
> Same comment elsewhere in the file. Perhaps we could add a helper that
> checks whether something is (plus (sp) (const_int N)) and if so returns
> the const_int (otherwise it returns null). It could then issue the
> sorry() diagnostic for SVE offsets.
It makes sense. The sorry() diagnostic for SVE offsets will be added.
> > + {
> > + fprintf (f, "\t.seh_save_%s x%ld, %ld\n",
> > + increment != 0 ? "regp_x" : "regp",
> > + min_regno,
> > + increment != 0 ? abs (increment) :
> > + seh_parallel_offset (pat, min_regno));
> > + }
>
> There seems to be an implicit assumption here that the two registers
> are consecutive, but that isn't necessarily the case. E.g.:
>
> void foo()
> {
> asm volatile ("" ::: "x19", "x22");
> }
>
> generates:
>
> .cfi_startproc
> stp x19, x22, [sp, -16]!
> .cfi_def_cfa_offset 16
> .cfi_offset 19, -16
> .cfi_offset 22, -8
> ldp x19, x22, [sp], 16
> .cfi_restore 22
> .cfi_restore 19
> .cfi_def_cfa_offset 0
> ret
Two nonconsecutive registers will be supported in v2.
> > +
> > + case MEM: // Save
> > + dest = XEXP (dest, 0);
> > + if (GET_CODE (dest) == PRE_DEC
> > + && CALLEE_SAVED_REG_NUMBER (REGNO (src))
> > + && XEXP (dest, 0) == stack_pointer_rtx)
> > + seh_emit_save (f, seh, src, INTVAL (XEXP (dest, 1)));
>
> PRE_DEC doesn't have a second operand. But wouldn't this call
> to seh_emit_save lose the pre-decrement anyway?
>
> I think the prologue save code would normally use PRE_MODIFY rather
> than PRE_DEC.
It looks like PRE_DEC is not needed anymore and will be removed.
> > +
> > + if (GET_CODE (pat) == SET)
> > + {
> > + rtx dest = SET_DEST (pat);
> > + if (GET_CODE (dest) == MEM && GET_CODE (XEXP (dest, 0)) == SCRATCH)
> > + return;
> > + }
>
> It looks like this hook is called for all instructions, but I would
> expect only RTX_FRAME_RELATED_P instructions to be relevant. As it stands,
> it looks like we could pick up stores of registers that aren't relevant
> to EH.
>
> For example, not every store of x19 to sp+16 is a prologue save and
> not every load of x19 from sp+16 is an epilogue restore.
A good point, RTX_FRAME_RELATED_P check will be used.
> > +
> > + case REG_CFA_EXPRESSION:
> > + case REG_CFA_REGISTER:
> > + case REG_CFA_ADJUST_CFA:
> > + case REG_CFA_OFFSET:
> > + related_exp_needed = false;
> > + break;
>
> Could you explain the handling of these notes? E.g. the code seems to
> have the effect of ignoring instructions that have REG_CFA_OFFSET notes,
> but I would have expected those to matter in a similar way to
> REG_FRAME_RELATED_EXPR.
In v2, these notes will not be ignored.
> Does the code support shrink-wrapping, or does it require all EH-relevant
> stores to happen in the prologue? If the latter, we should disable
> shrink-wrapping of the relevant components.
Thanks for mentioning this. shrink-wrapping is not supported and will be
disabled.
Thank you for this valuable review. It has been some time since this review,
as the main focus was on upstreaming binutils changes for SEH, however it is
very helpful.
Regards,
Evgeny
