[clang backend] In AArch64's DataLayout, specify a minimum function alignment of 4. #90702

dougsonos · 2024-05-01T04:46:59Z

This addresses an issue where the explicit alignment of 2 (for C++ ABI reasons) was being propagated to the back end and causing under-aligned functions (in special sections). (#90358)

This is an alternate approach suggested by @efriedma-quic in PR #90415.

~~(There are a few failing tests. One I looked at was checking for the explicit alignment of 2, which no longer appears in AArch64 due to this change.)~~

This addresses an issue where the explicit alignment of 2 (for C++ ABI reasons) was being propagated to the back end and causing under-aligned functions (in special sections). (llvm#90358)

github-actions · 2024-05-01T04:47:15Z

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.

llvmbot · 2024-05-01T04:47:43Z

@llvm/pr-subscribers-backend-aarch64

@llvm/pr-subscribers-llvm-ir

Author: Doug Wyatt (dougsonos)

Changes

This addresses an issue where the explicit alignment of 2 (for C++ ABI reasons) was being propagated to the back end and causing under-aligned functions (in special sections). (#90358)

This is an alternate approach suggested by @efriedma-quic in PR #90415.

(There are a few failing tests. One I looked at was checking for the explicit alignment of 2, which no longer appears in AArch64 due to this change.)

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

3 Files Affected:

(modified) clang/lib/Basic/Targets/AArch64.cpp (+6-6)
(modified) llvm/lib/IR/AutoUpgrade.cpp (+8)
(modified) llvm/lib/Target/AArch64/AArch64TargetMachine.cpp (+4-4)

diff --git a/clang/lib/Basic/Targets/AArch64.cpp b/clang/lib/Basic/Targets/AArch64.cpp
index c8d243a8fb7aea..a5dd803f636b90 100644
--- a/clang/lib/Basic/Targets/AArch64.cpp
+++ b/clang/lib/Basic/Targets/AArch64.cpp
@@ -1480,11 +1480,11 @@ AArch64leTargetInfo::AArch64leTargetInfo(const llvm::Triple &Triple,
 void AArch64leTargetInfo::setDataLayout() {
   if (getTriple().isOSBinFormatMachO()) {
     if(getTriple().isArch32Bit())
-      resetDataLayout("e-m:o-p:32:32-i64:64-i128:128-n32:64-S128", "_");
+      resetDataLayout("e-m:o-p:32:32-Fn32-i64:64-i128:128-n32:64-S128", "_");
     else
-      resetDataLayout("e-m:o-i64:64-i128:128-n32:64-S128", "_");
+      resetDataLayout("e-m:o-Fn32-i64:64-i128:128-n32:64-S128", "_");
   } else
-    resetDataLayout("e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128");
+    resetDataLayout("e-m:e-Fn32-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128");
 }
 
 void AArch64leTargetInfo::getTargetDefines(const LangOptions &Opts,
@@ -1507,7 +1507,7 @@ void AArch64beTargetInfo::getTargetDefines(const LangOptions &Opts,
 
 void AArch64beTargetInfo::setDataLayout() {
   assert(!getTriple().isOSBinFormatMachO());
-  resetDataLayout("E-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128");
+  resetDataLayout("E-m:e-Fn32-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128");
 }
 
 WindowsARM64TargetInfo::WindowsARM64TargetInfo(const llvm::Triple &Triple,
@@ -1530,8 +1530,8 @@ WindowsARM64TargetInfo::WindowsARM64TargetInfo(const llvm::Triple &Triple,
 
 void WindowsARM64TargetInfo::setDataLayout() {
   resetDataLayout(Triple.isOSBinFormatMachO()
-                      ? "e-m:o-i64:64-i128:128-n32:64-S128"
-                      : "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128",
+                      ? "e-m:o-Fn32-i64:64-i128:128-n32:64-S128"
+                      : "e-m:w-p:64:64-Fn32-i32:32-i64:64-i128:128-n32:64-S128",
                   Triple.isOSBinFormatMachO() ? "_" : "");
 }
 
diff --git a/llvm/lib/IR/AutoUpgrade.cpp b/llvm/lib/IR/AutoUpgrade.cpp
index 634b2dd5119e8d..eed946dc36580e 100644
--- a/llvm/lib/IR/AutoUpgrade.cpp
+++ b/llvm/lib/IR/AutoUpgrade.cpp
@@ -5387,6 +5387,14 @@ std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
     return Res;
   }
 
+  // AArch64 data layout upgrades.
+  if (T.isAArch64()) {
+    // Add "-Fn32"
+    if (!DL.contains("-Fn32"))
+      Res.append("-Fn32");
+    return Res;
+  }
+
   if (!T.isX86())
     return Res;
 
diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
index 7ef78cbba352a5..4ff5fb94162a93 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -275,15 +275,15 @@ static std::string computeDataLayout(const Triple &TT,
                                      bool LittleEndian) {
   if (TT.isOSBinFormatMachO()) {
     if (TT.getArch() == Triple::aarch64_32)
-      return "e-m:o-p:32:32-i64:64-i128:128-n32:64-S128";
-    return "e-m:o-i64:64-i128:128-n32:64-S128";
+      return "e-m:o-p:32:32-Fn32-i64:64-i128:128-n32:64-S128";
+    return "e-m:o-Fn32-i64:64-i128:128-n32:64-S128";
   }
   if (TT.isOSBinFormatCOFF())
-    return "e-m:w-p:64:64-i32:32-i64:64-i128:128-n32:64-S128";
+    return "e-m:w-p:64:64-Fn32-i32:32-i64:64-i128:128-n32:64-S128";
   std::string Endian = LittleEndian ? "e" : "E";
   std::string Ptr32 = TT.getEnvironment() == Triple::GNUILP32 ? "-p:32:32" : "";
   return Endian + "-m:e" + Ptr32 +
-         "-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128";
+         "-Fn32-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128";
 }
 
 static StringRef computeDefaultCPU(const Triple &TT, StringRef CPU) {

efriedma-quic

This looks correct.

I'd expect regression tests for the autoupgrade, and a test that clang isn't generating the alignment markings anymore.

llvm/lib/IR/AutoUpgrade.cpp

auto-upgrade simplest.

- CodeGen/target-data.c - CodeGen/coff-aarch64-type-sizes.c - CodeGen/aarch64-type-sizes.c - CodeGen/CodeGenCXX/member-alignment.cpp - OpenMP/distribute_parallel_for_num_threads_codegen.cpp

dougsonos · 2024-05-01T14:56:49Z

I'd expect regression tests for the autoupgrade, and a test that clang isn't generating the alignment markings anymore.

I just pushed:

the -Fn32 is at the end of the data layout strings
the 6 tests that failed for me locally (testing only AArch64) are fixed

That includes a change to CodeGen/CodeGenCXX/member-alignment.cpp, which checks for alignment / no alignment on C++ methods, depending on target.

~~I'm not clear on when the autoupgrade runs and how to test it. Is there an example I can follow?~~ EDIT: I found DataLayoutUpgradeTest.cpp...

Thanks!

DataLayoutUpgradeTest: adapt test to this PR's change.

dougsonos · 2024-05-01T17:06:45Z

Linux x86_64 has one test failure: RISCV/unnamed-sym-no-entry.c . AFAICT it looks unrelated.

fhahn · 2024-05-02T08:59:19Z

clang/lib/Basic/Targets/AArch64.cpp

@@ -1480,11 +1480,11 @@ AArch64leTargetInfo::AArch64leTargetInfo(const llvm::Triple &Triple,
 void AArch64leTargetInfo::setDataLayout() {
  if (getTriple().isOSBinFormatMachO()) {
    if(getTriple().isArch32Bit())
-      resetDataLayout("e-m:o-p:32:32-i64:64-i128:128-n32:64-S128", "_");
+      resetDataLayout("e-m:o-p:32:32-i64:64-i128:128-n32:64-S128-Fn32", "_");


Just to double check @TNorthover, is an alignment of 32 OK for arm64_32?

Yep. CPU would hard-fault if that was violated. All instructions are 4 bytes and must be aligned.

Is this true even if the cpu was executing thumb2 instructions in AArch32 mode?

Thumb2 function pointers aren't aligned... but from LLVM's perspective it's a completely different target, so it's not relevant.

fhahn

LGTM, thanks!

efriedma-quic

LGTM

efriedma-quic · 2024-05-05T20:31:16Z

Which email address do you want the commit associated with? The GitHub "squash and merge" button wants to attribute it to your "sonosphere.com" address, but it looks like the commits themselves are using an "apple.com" address.

dougsonos · 2024-05-05T21:15:39Z

Which email address do you want the commit associated with? The GitHub "squash and merge" button wants to attribute it to your "sonosphere.com" address, but it looks like the commits themselves are using an "apple.com" address.

Well neither will last forever, but hopefully sonosphere.com lasts a bit longer, so use that one please :) Thanks

github-actions · 2024-05-06T02:05:31Z

@dougsonos Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested
by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as
the builds can include changes from many authors. It is not uncommon for your
change to be included in a build that fails due to someone else's changes, or
infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself.
This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

llvmbot · 2024-05-06T02:25:08Z

/cherry-pick ddecada

Error: Command failed due to missing milestone.

llvmbot · 2024-05-06T02:25:40Z

/cherry-pick ddecada

Error: Command failed due to missing milestone.

@efriedma-quic

…lignment of 4. (llvm#90702) This addresses an issue where the explicit alignment of 2 (for C++ ABI reasons) was being propagated to the back end and causing under-aligned functions (in special sections). This is an alternate approach suggested by @efriedma-quic in PR llvm#90415. Fixes llvm#90358 (cherry picked from commit ddecada)

@efriedma-quic

…lignment of 4. (llvm#90702) This addresses an issue where the explicit alignment of 2 (for C++ ABI reasons) was being propagated to the back end and causing under-aligned functions (in special sections). This is an alternate approach suggested by @efriedma-quic in PR llvm#90415. Fixes llvm#90358

In AArch64's DataLayout, specify a minimum function alignment of 4.

4c312af

This addresses an issue where the explicit alignment of 2 (for C++ ABI reasons) was being propagated to the back end and causing under-aligned functions (in special sections). (llvm#90358)

llvmbot added clang Clang issues not falling into any other category backend:AArch64 clang:frontend Language frontend issues, e.g. anything involving "Sema" llvm:ir labels May 1, 2024

dougsonos mentioned this pull request May 1, 2024

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

Closed

efriedma-quic reviewed May 1, 2024

View reviewed changes

llvm/lib/IR/AutoUpgrade.cpp Show resolved Hide resolved

Doug Wyatt added 2 commits May 1, 2024 07:27

Move "-Fn32" to the end of the data layout strings, to make the

5a2b9e1

auto-upgrade simplest.

Fix tests:

43ca2b2

- CodeGen/target-data.c - CodeGen/coff-aarch64-type-sizes.c - CodeGen/aarch64-type-sizes.c - CodeGen/CodeGenCXX/member-alignment.cpp - OpenMP/distribute_parallel_for_num_threads_codegen.cpp

llvmbot added the clang:openmp OpenMP related changes to Clang label May 1, 2024

AutoUpgrade: don't try to upgrade an empty string.

aec3a3d

DataLayoutUpgradeTest: adapt test to this PR's change.

More correct DataLayoutUpgradeTests

aad17e8

fhahn requested review from fhahn, TNorthover and davemgreen May 2, 2024 08:54

fhahn reviewed May 2, 2024

View reviewed changes

fhahn approved these changes May 2, 2024

View reviewed changes

Merge branch 'main' into min_sect_align_2

5002204

efriedma-quic approved these changes May 5, 2024

View reviewed changes

efriedma-quic merged commit ddecada into llvm:main May 6, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[clang backend] In AArch64's DataLayout, specify a minimum function alignment of 4. #90702

[clang backend] In AArch64's DataLayout, specify a minimum function alignment of 4. #90702

dougsonos commented May 1, 2024 •

edited

Loading

github-actions bot commented May 1, 2024

llvmbot commented May 1, 2024 •

edited

Loading

efriedma-quic left a comment

dougsonos commented May 1, 2024 •

edited

Loading

dougsonos commented May 1, 2024

fhahn May 2, 2024

TNorthover May 2, 2024

AZero13 May 6, 2024

efriedma-quic May 6, 2024

fhahn left a comment

efriedma-quic left a comment

efriedma-quic commented May 5, 2024

dougsonos commented May 5, 2024

github-actions bot commented May 6, 2024

llvmbot commented May 6, 2024

llvmbot commented May 6, 2024

[clang backend] In AArch64's DataLayout, specify a minimum function alignment of 4. #90702

[clang backend] In AArch64's DataLayout, specify a minimum function alignment of 4. #90702

Conversation

dougsonos commented May 1, 2024 • edited Loading

github-actions bot commented May 1, 2024

llvmbot commented May 1, 2024 • edited Loading

efriedma-quic left a comment

Choose a reason for hiding this comment

dougsonos commented May 1, 2024 • edited Loading

dougsonos commented May 1, 2024

fhahn May 2, 2024

Choose a reason for hiding this comment

TNorthover May 2, 2024

Choose a reason for hiding this comment

AZero13 May 6, 2024

Choose a reason for hiding this comment

efriedma-quic May 6, 2024

Choose a reason for hiding this comment

fhahn left a comment

Choose a reason for hiding this comment

efriedma-quic left a comment

Choose a reason for hiding this comment

efriedma-quic commented May 5, 2024

dougsonos commented May 5, 2024

github-actions bot commented May 6, 2024

llvmbot commented May 6, 2024

llvmbot commented May 6, 2024

dougsonos commented May 1, 2024 •

edited

Loading

llvmbot commented May 1, 2024 •

edited

Loading

dougsonos commented May 1, 2024 •

edited

Loading