This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][Windows] Add MC support for save_any_reg.
ClosedPublic

Authored by efriedma on Oct 6 2022, 5:29 PM.

Details

Summary

Representing this as 12 separate operations is a bit ugly, but trying to represent the different modes using a bitfield seemed worse.

Diff Detail

Event Timeline

efriedma created this revision.Oct 6 2022, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 5:29 PM
efriedma requested review of this revision.Oct 6 2022, 5:29 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 6 2022, 5:29 PM

Representing this as 12 separate operations is a bit ugly, but trying to represent the different modes using a bitfield seemed worse.

Yup, and there's a fairly limited amount of info you can store in the WinEH::Instruction class today, so I guess this looks reasonable.

Technically there's no need to have the assembly level directives mirror exactly the same 12 separate opcodes - we could consider e.g. checking whether the provided register name is in the range of x0-x30, d0-d31 or q0-q31 and pick the right opcode based on that - that would reduce the number of assembly directives down to 4 instead of 12. (That's the only aspect of this which is a public interface.) What do you think of that? Internally throughout the rest of the interfaces (AArch64TargetStreamer) it's probably easiest to keep all 12 separate entrypoints in any case.

llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
6104

Nit: Missing space after ==

That would be four directives, "seh_save_any_reg", seh_save_any_reg_p", "seh_save_any_reg_x", and "seh_save_any_reg_px"? I guess that works, although I'll have to reimplement the parseRegisterInRange() logic.

That would be four directives, "seh_save_any_reg", seh_save_any_reg_p", "seh_save_any_reg_x", and "seh_save_any_reg_px"? I guess that works, although I'll have to reimplement the parseRegisterInRange() logic.

Exactly.

I'm not entirely dead set on it, but I think it could be a nicer experience for the assembly directives and at least worth evaluating.

efriedma updated this revision to Diff 467279.Oct 12 2022, 3:34 PM

Reduce to 4 user-visible asm directives. (Still 12 operations internally.)

mstorsjo accepted this revision.Oct 12 2022, 11:57 PM

LGTM, thanks! This does look nicer to me in the end.

BTW, the process for contributing to the public docs on the unwind info format went really smoothly for me this time, so getting this opcode documented should hopefully go well too.

This revision is now accepted and ready to land.Oct 12 2022, 11:57 PM
This revision was landed with ongoing or failed builds.Oct 18 2022, 11:45 AM
This revision was automatically updated to reflect the committed changes.