[clang][ARM] disable frame pointers by default for bare metal ARM targets #117140

stuij · 2024-11-21T10:36:41Z

because:

This brings Clang in line with GCC for which this is the default for ARM
It frees up a register, so performance increase, especially on Thumb/6-M
It will decrease code size

…gets because: - This brings Clang in line with GCC for which this is the default for ARM - It frees up a register, so performance increase, especially on Thumb/6-M - It will also decrease code size

llvmbot · 2024-11-21T10:37:19Z

@llvm/pr-subscribers-backend-arm
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Ties Stuij (stuij)

Changes

because:

This brings Clang in line with GCC for which this is the default for ARM
It frees up a register, so performance increase, especially on Thumb/6-M
It will decrease code size

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

4 Files Affected:

(modified) clang/lib/Driver/ToolChains/BareMetal.cpp (+7-1)
(modified) clang/lib/Driver/ToolChains/BareMetal.h (+2)
(modified) clang/lib/Driver/ToolChains/CommonArgs.cpp (+5)
(modified) clang/test/Driver/frame-pointer-elim.c (+29)

diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp b/clang/lib/Driver/ToolChains/BareMetal.cpp
index f9a73f60973e4c..13b510e7e70994 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -128,8 +128,11 @@ BareMetal::BareMetal(const Driver &D, const llvm::Triple &Triple,
   }
 }
 
+namespace clang {
+namespace driver {
+namespace toolchains {
 /// Is the triple {arm,armeb,thumb,thumbeb}-none-none-{eabi,eabihf} ?
-static bool isARMBareMetal(const llvm::Triple &Triple) {
+bool isARMBareMetal(const llvm::Triple &Triple) {
   if (Triple.getArch() != llvm::Triple::arm &&
       Triple.getArch() != llvm::Triple::thumb &&
       Triple.getArch() != llvm::Triple::armeb &&
@@ -148,6 +151,9 @@ static bool isARMBareMetal(const llvm::Triple &Triple) {
 
   return true;
 }
+} // namespace clang
+} // namespace driver
+} // namespace clang
 
 /// Is the triple {aarch64.aarch64_be}-none-elf?
 static bool isAArch64BareMetal(const llvm::Triple &Triple) {
diff --git a/clang/lib/Driver/ToolChains/BareMetal.h b/clang/lib/Driver/ToolChains/BareMetal.h
index b385c8cf76aab0..ae09bcedd78a28 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.h
+++ b/clang/lib/Driver/ToolChains/BareMetal.h
@@ -19,6 +19,8 @@ namespace driver {
 
 namespace toolchains {
 
+bool isARMBareMetal(const llvm::Triple &Triple);
+
 class LLVM_LIBRARY_VISIBILITY BareMetal : public ToolChain {
 public:
   BareMetal(const Driver &D, const llvm::Triple &Triple,
diff --git a/clang/lib/Driver/ToolChains/CommonArgs.cpp b/clang/lib/Driver/ToolChains/CommonArgs.cpp
index 8d977149e62485..8d54d0a8649cc9 100644
--- a/clang/lib/Driver/ToolChains/CommonArgs.cpp
+++ b/clang/lib/Driver/ToolChains/CommonArgs.cpp
@@ -19,6 +19,7 @@
 #include "Arch/SystemZ.h"
 #include "Arch/VE.h"
 #include "Arch/X86.h"
+#include "BareMetal.h"
 #include "HIPAMD.h"
 #include "Hexagon.h"
 #include "MSP430.h"
@@ -151,6 +152,10 @@ static bool useFramePointerForTargetByDefault(const llvm::opt::ArgList &Args,
     }
   }
 
+  if (toolchains::isARMBareMetal(Triple)) {
+    return false;
+  }
+
   return true;
 }
 
diff --git a/clang/test/Driver/frame-pointer-elim.c b/clang/test/Driver/frame-pointer-elim.c
index cdedcc7ae4c89f..667c47b34bc703 100644
--- a/clang/test/Driver/frame-pointer-elim.c
+++ b/clang/test/Driver/frame-pointer-elim.c
@@ -162,5 +162,34 @@
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: not %clang -### --target=riscv64-linux-android -mbig-endian -O1 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+
+// On ARM backend bare metal targets, frame pointer is omitted
+// RUN: %clang -### --target=arm-arm-none-eabi -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
+// RUN: %clang -### --target=arm-arm-none-eabihf -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
+// RUN: %clang -### --target=arm-arm-none-eabi -S -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
+// RUN: %clang -### --target=arm-arm-none-eabihf -S -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
+// RUN: %clang -### --target=arm-arm-none-eabi -S -O1 %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
+// RUN: %clang -### --target=arm-arm-none-eabihf -S -O1 %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NONE %s
+// RUN: %clang -### --target=arm-arm-none-eabi -S -O1 -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
+// RUN: %clang -### --target=arm-arm-none-eabihf -S -O1 -fno-omit-frame-pointer %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-ALL %s
+
+// AArch64 bare metal targets behave like hosted targets
+// RUN: %clang -### --target=aarch64-none-elf -S %s 2>&1 |  \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=aarch64-none-elf -S -O1 %s 2>&1 |  \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=aarch64-none-elf -S -fno-omit-frame-pointer %s 2>&1 |  \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+// RUN: %clang -### --target=aarch64-none-elf -S -O1 -fno-omit-frame-pointer %s 2>&1 |  \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
+
 void f0() {}
 void f1() { f0(); }

stuij · 2024-11-21T10:39:06Z

This change was discussed in the LLVM Embedded Toolchains working group sync up two weeks ago, and at the time people were in agreement that it made sense:

https://discourse.llvm.org/t/llvm-embedded-toolchains-working-group-sync-up/63270/73#p-334070-peter-frame-pointers-on-or-off-by-default-7

github-actions · 2024-11-21T10:40:30Z

✅ With the latest revision this PR passed the C/C++ code formatter.

clang/lib/Driver/ToolChains/BareMetal.cpp

clang/lib/Driver/ToolChains/CommonArgs.cpp

nickdesaulniers

What are your thoughts on moving the definition into the BareMetal.h header, then adding cases for arm, armeb, thumb, and thumbeb to the existing getArch switch in CommonArgs.cpp#L85? Then we should be able to inline isARMBareMetal and elide the getArch checks in it.

stuij · 2024-11-25T17:21:24Z

I feel that in useFramePointerForTargetByDef fn, in general things are predicated first on platform and then on arch. That L85 arch switch statement seems to serve as a "these arches don't do anything special" early return.

It's up to interpretation, but putting the different Arm variants there seems less elegant to me (that might not be saying much..). I'd say let's leave it alone for now, and if the bare metal logic gets more impenetrable at some point, refactor.

also add a few more non-apple embedded tests and make isARMBareMetal slightly more elegant

To make it clearer what the fn is testing for.

clang/lib/Driver/ToolChains/BareMetal.cpp

asb · 2024-12-09T04:03:10Z

I think this change in behaviour deserves an entry in clang/docs/ReleaseNotes.rst

Pull request #109628 automatically turned on frame pointers for leaf functions in Clang when frame pointers are enabled. This triggered a latent bug in picolibc for armv4t and armv5te, which consequentially broke our tests for them. There is currently a pull request that fixes this bug (picolibc/picolibc#897), but we don't have to wait for this to merge to reinstate the tests, because we recently changed the behaviour of Clang to by default omit frame pointers altogether (llvm/llvm-project#117140), and that is how picolibc is built now. In general the advice is that you shouldn't build AArch32 targets with frame pointers enabled for various reasons.

[clang][ARM] disable frame pointers by default for bare metal ARM tar…

4a85a0c

…gets because: - This brings Clang in line with GCC for which this is the default for ARM - It frees up a register, so performance increase, especially on Thumb/6-M - It will also decrease code size

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Nov 21, 2024

tbaederr reviewed Nov 21, 2024

View reviewed changes

clang/lib/Driver/ToolChains/BareMetal.cpp Outdated Show resolved Hide resolved

fix clang -> toolchains closing brace comment mislabel

51a5d72

stuij requested review from pratlucas, john-brawn-arm and nickdesaulniers November 21, 2024 15:15

nickdesaulniers requested review from jroelofs and petrhosek November 21, 2024 16:24

nickdesaulniers reviewed Nov 21, 2024

View reviewed changes

clang/lib/Driver/ToolChains/CommonArgs.cpp Outdated Show resolved Hide resolved

Remove braces around single statement if body

9aeb61f

jroelofs reviewed Nov 21, 2024

View reviewed changes

clang/lib/Driver/ToolChains/CommonArgs.cpp Outdated Show resolved Hide resolved

nickdesaulniers reviewed Nov 21, 2024

View reviewed changes

stuij added 2 commits November 26, 2024 10:41

Add Apple embedded tests

d953a8d

also add a few more non-apple embedded tests and make isARMBareMetal slightly more elegant

satisfy clang-format

b972b8b

stuij requested review from jroelofs and nickdesaulniers November 28, 2024 10:32

Rename isARMBareMetal -> isArmEABIBareMetal

26c854f

To make it clearer what the fn is testing for.

nickdesaulniers approved these changes Dec 2, 2024

View reviewed changes

petrhosek reviewed Dec 2, 2024

View reviewed changes

clang/lib/Driver/ToolChains/BareMetal.cpp Outdated Show resolved Hide resolved

Move isARMEABIBareMetal to clang/lib/Driver/ToolChains/Arch/ARM.cpp

cc3b138

llvmbot added the backend:ARM label Dec 3, 2024

stuij requested a review from petrhosek December 5, 2024 10:45

petrhosek approved these changes Dec 5, 2024

View reviewed changes

stuij merged commit 2f4eac6 into llvm:main Dec 6, 2024
9 checks passed

stuij deleted the fp-off branch December 6, 2024 14:05

stuij mentioned this pull request Dec 20, 2024

Reinstate armv4t and armv5te compiler_rt and libcxx tests ARM-software/LLVM-embedded-toolchain-for-Arm#614

Merged

stuij mentioned this pull request Jan 14, 2025

[clang] document that by default FP turned off for ARM baremetal #122881

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang][ARM] disable frame pointers by default for bare metal ARM targets #117140

[clang][ARM] disable frame pointers by default for bare metal ARM targets #117140

Uh oh!

stuij commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 21, 2024 •

edited

Loading

Uh oh!

stuij commented Nov 21, 2024

Uh oh!

github-actions bot commented Nov 21, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers left a comment

Uh oh!

stuij commented Nov 25, 2024

Uh oh!

Uh oh!

Uh oh!

asb commented Dec 9, 2024

Uh oh!

Uh oh!

[clang][ARM] disable frame pointers by default for bare metal ARM targets #117140

[clang][ARM] disable frame pointers by default for bare metal ARM targets #117140

Uh oh!

Conversation

stuij commented Nov 21, 2024

Uh oh!

llvmbot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuij commented Nov 21, 2024

Uh oh!

github-actions bot commented Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nickdesaulniers left a comment

Choose a reason for hiding this comment

Uh oh!

stuij commented Nov 25, 2024

Uh oh!

Uh oh!

Uh oh!

asb commented Dec 9, 2024

Uh oh!

Uh oh!

llvmbot commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024 •

edited

Loading