This is an archive of the discontinued LLVM Phabricator instance.

[COFF, ARM64] Align global symbol by size for ARM64 MSVC ABI
ClosedPublic

Authored by TomTan on Apr 27 2019, 8:35 AM.

Details

Summary

According to alignment section in below ARM64 ABI document, MSVC could increase alignment of global data based on its total size. Clang doesn't do this. Compile the same symbol into different alignments by Clang and MSVC could cause link error because some instruction encodings, like 64-bit LDR/STR with immediate, require the target to be 8 bytes aligned, and linker could choose code stream with such LDR/STR instruction from MSVC and 4 bytes aligned data from Clang into final image, which actually cannot be linked together (see https://bugs.llvm.org/show_bug.cgi?id=41506 for more details).

https://docs.microsoft.com/en-us/cpp/build/arm64-windows-abi-conventions?view=vs-2019#alignment

Diff Detail

Repository
rL LLVM

Event Timeline

TomTan created this revision.Apr 27 2019, 8:35 AM

I was going to ask if local variables are also supposed to be aligned this way... but I guess there's no way for standard C++ code to tell without explicitly making weird alignment assumptions, so let's not worry about that.

This should do the right thing, as far as I can tell.

It probably would make sense to refactor the arm64-specific code into a TargetInfo API... or at least the triple check. Not sure what that would look like; maybe virtual uint64_t getMinGlobalAlign(uint64_t TypeSize)? Or maybe should just be "bool useMicrosoftGlobalAlign()" and keep the main logic here, if the rule is the same for multiple Windows targets.

Needs a testcase in test/CodeGenCXX/ demonstrating the alignment of the emitted global in various cases.

Since this is target-specific, is it right to put this here ?

TomTan updated this revision to Diff 197208.Apr 29 2019, 3:54 PM

Added test cases and also merged this alignment adjustment to getMinGlobalAlign in MicrosoftARM64TargetInfo.

efriedma accepted this revision.May 1 2019, 4:46 PM

LGTM

This revision is now accepted and ready to land.May 1 2019, 4:46 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 1 2019, 5:37 PM