[CodeGen] Don't allow function alignment less than the target's minimum

[dougsonos]

See #90358

The C++ ABI forces all class methods to have an explicit alignment of 2. This ends up overriding MachineFunction's alignment (on arm64, 4) on functions in special text sections. Add a check: don't align code at less than the target's minimum function alignment.

(Are there other objects which can occur in text sections and which should be allowed to be aligned at smaller than the minimum function alignment?)

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[inclyc]

Thanks for contributing LLVM! Would you please add test cases to cover your changed lines?

[dougsonos]

Thanks for contributing LLVM! Would you please add test cases to cover your changed lines?

I will work on that. Any pointers on a general testing approach here?

Idea: construct bitcode source files which mimic what Clang generates, then convert them to assembly language (for some particular target...?) and check the alignment directives.

EDIT: That seems to have worked, at least locally on macOS. Not sure yet how non-Mac platforms will interpret the section directive in the test case though.

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: Doug Wyatt (dougsonos)

Changes

See #90358

The C++ ABI forces all class methods to have an explicit alignment of 2. This ends up overriding MachineFunction's alignment (on arm64, 4) on functions in special text sections. Add a check: don't align code at less than the target's minimum function alignment.

(Are there other objects which can occur in text sections and which should be allowed to be aligned at smaller than the minimum function alignment?)

---

Full diff: https://github.com/llvm/llvm-project/pull/90415.diff

2 Files Affected:

• (modified) llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp (+11-3)
• (added) llvm/test/CodeGen/AArch64/func-alignment.ll (+19)

diff --git a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
index 721d144d7f4c67..c29f9aa97f0165 100644
--- a/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
+++ b/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp
@@ -3147,9 +3147,17 @@ void AsmPrinter::emitAlignment(Align Alignment, const GlobalObject *GV,
 
   if (getCurrentSection()->getKind().isText()) {
     const MCSubtargetInfo *STI = nullptr;
-    if (this->MF)
-      STI = &getSubtargetInfo();
-    else
+    if (this->MF) {
+      // Don't allow the possibly smaller alignment of the GV
+      // (e.g. 2 for a C++ method) to reduce a function's alignment
+      // below the target's minimum function alignment. (#90358)
+      const TargetSubtargetInfo *TSI = &MF->getSubtarget();
+      const Align MinAlign =
+          TSI->getTargetLowering()->getMinFunctionAlignment();
+      if (Alignment < MinAlign)
+        Alignment = MinAlign;
+      STI = TSI;
+    } else
       STI = TM.getMCSubtargetInfo();
     OutStreamer->emitCodeAlignment(Alignment, STI, MaxBytesToEmit);
   } else
diff --git a/llvm/test/CodeGen/AArch64/func-alignment.ll b/llvm/test/CodeGen/AArch64/func-alignment.ll
new file mode 100644
index 00000000000000..0860578065bcd1
--- /dev/null
+++ b/llvm/test/CodeGen/AArch64/func-alignment.ll
@@ -0,0 +1,19 @@
+; RUN: llc -mtriple=aarch64 -O0 -fast-isel < %s | FileCheck %s
+
+; Verify that Clang's C++ ABI alignment for functions of 2 does not
+; result in underaligned functions when they are in special sections.
+; (#90415)
+
+define void @noSection() align 2 {
+; CHECK:	.p2align	2
+entry:
+  ret void
+}
+
+define void @withSection() section "__TEXT,__foo,regular,pure_instructions" align 2 {
+; CHECK:	.p2align	2
+entry:
+  ret void
+}
+
+

[efriedma-quic]

Alignment of functions is already confusing enough... this change makes it more confusing. We should compute the alignment of a function exactly once, not compute it, then override it later.

Ideally, we should just fix clang so it doesn't make this request in the first place. The relevant code is in

llvm-project/clang/lib/CodeGen/CodeGenModule.cpp

Line 2492 in 6566ffd

if (isa<CXXMethodDecl>(D) && F->getPointerAlignment(getDataLayout()) < 2)

... but really, we should just fix the datalayout so getPointerAlignment() works correctly. (See https://reviews.llvm.org/D57335 )

CC @daltenty @hubert-reinterpretcast

[dougsonos]

Ideally, we should just fix clang so it doesn't make this request in the first place.

OK ... I verified that this underalignment only happens on C++ methods, where this code (right after your reference) causes Clang to emit a specific alignment on the function where otherwise it would not. Functions where Clang does not specify alignment do not end up underaligned.

  if (getTarget().getCXXABI().areMemberFunctionsAligned()) {
    if (isa<CXXMethodDecl>(D) && F->getPointerAlignment(getDataLayout()) < 2)
      F->setAlignment(std::max(llvm::Align(2), F->getAlign().valueOrOne()));
  }

I see that in the referenced PR, the arm (not arm64) and ppc targets were updated to have a function pointer alignment of 8. Looking at an object file in my local Clang build, it's apparent that functions are only aligned to 4 bytes, so it seems unnecessary to align them to 8.

If we want to fix things here, it seems we have to look at that same TargetLowering getMinFunctionAlignment(), no?

Thanks for the reply.

[efriedma-quic]

On 32-bit Arm, function pointer alignment is only 1 byte (8 bits as spelled in the datalayout) because of the way Thumb works. On 64-bit, 4 bytes should be right. (And on PPC, it depends on the ABI.) I've been meaning to extend the function pointer alignment specification to other targets, but didn't get around to it. Didn't realize it affected this code.

getPointerAlignment queries the datalayout already, so if the datalayout specifies the alignment, the existing code should just work.

The alternative here is to add some code to clang/lib/Basic/Targets to specify function pointer alignment... (You can't call TargetLowering from clang; clang IR generation is intentionally separated from the backend.)

[dougsonos]

On 32-bit Arm, function pointer alignment is only 1 byte (8 bits as spelled in the datalayout) because of the way Thumb works. On 64-bit, 4 bytes should be right. (And on PPC, it depends on the ABI.) I've been meaning to extend the function pointer alignment specification to other targets, but didn't get around to it. Didn't realize it affected this code.

getPointerAlignment queries the datalayout already, so if the datalayout specifies the alignment, the existing code should just work.

The alternative here is to add some code to clang/lib/Basic/Targets to specify function pointer alignment... (You can't call TargetLowering from clang; clang IR generation is intentionally separated from the backend.)

My mistake, I read the "Fi8" in the ARM/PPC data layouts as 8 bytes, but of course it's 8 bits.

I could add "Fi32" to the AArch64 data layouts. (Any pointers to how to do this most cleanly?)

(newbie Q: Should I open a different PR or repurpose this one?)

[efriedma-quic]

I could add "Fi32" to the AArch64 data layouts. (Any pointers to how to do this most cleanly?)

Should be Fn32. ("i" is only for targets where function pointers aren't simple pointers to code... like Arm and PPC.)

The datalayout is in two places: clang/lib/Basic/Targets/AArch64.cpp, and llvm/lib/Target/AArch64/AArch64TargetMachine.cpp . Then, ideally, you update UpgradeDataLayoutString in llvm/lib/IR/AutoUpgrade.cpp so auto-upgraded IR inherits the right behavior.

(newbie Q: Should I open a different PR or repurpose this one?)

In this case, I'd create a new PR because the new approach is so much different.

[dougsonos]

In this case, I'd create a new PR because the new approach is so much different.

Thanks. I opened #90702

[dougsonos]

Closing in favor of #90702