[ARM] Introduce -mtp=auto and make it the default #128728

Zhenhang1213 · 2025-02-25T15:27:03Z

This adds a new value auto to the possible values of the existing -mtp= clang option which controls how the thread pointer is found. auto means the same as soft if the target architecture doesn't support a hardware thread pointer at all; otherwise it means the same as cp15.

This behavior is the default in gcc version 7.3.0 and later. The new auto option is therefore also the default in clang, so this change aligns clang with gcc.

llvmbot · 2025-02-25T15:28:40Z

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

@llvm/pr-subscribers-clang

Author: Austin (Zhenhang1213)

Changes

Fix #123864, resolved of the different form behavior of mtp

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

5 Files Affected:

(modified) clang/include/clang/Driver/Options.td (+1-1)
(modified) clang/lib/Driver/ToolChains/Arch/ARM.cpp (+5-1)
(modified) clang/lib/Driver/ToolChains/Arch/ARM.h (+1)
(modified) clang/lib/Driver/ToolChains/Clang.cpp (+1-1)
(modified) clang/test/Driver/arm-thread-pointer.c (+5-1)

diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index e521cbf678d93..2bd6076bea5d4 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -4711,7 +4711,7 @@ def mexecute_only : Flag<["-"], "mexecute-only">, Group<m_arm_Features_Group>,
 def mno_execute_only : Flag<["-"], "mno-execute-only">, Group<m_arm_Features_Group>,
   HelpText<"Allow generation of data access to code sections (ARM only)">;
 let Flags = [TargetSpecific] in {
-def mtp_mode_EQ : Joined<["-"], "mtp=">, Group<m_arm_Features_Group>, Values<"soft,cp15,tpidrurw,tpidruro,tpidrprw,el0,el1,el2,el3,tpidr_el0,tpidr_el1,tpidr_el2,tpidr_el3,tpidrro_el0">,
+def mtp_mode_EQ : Joined<["-"], "mtp=">, Group<m_arm_Features_Group>, Values<"soft,cp15,tpidrurw,tpidruro,tpidrprw,el0,el1,el2,el3,tpidr_el0,tpidr_el1,tpidr_el2,tpidr_el3,tpidrro_el0,auto">,
   HelpText<"Thread pointer access method. "
            "For AArch32: 'soft' uses a function call, or 'tpidrurw', 'tpidruro' or 'tpidrprw' use the three CP15 registers. 'cp15' is an alias for 'tpidruro'. "
            "For AArch64: 'tpidr_el0', 'tpidr_el1', 'tpidr_el2', 'tpidr_el3' or 'tpidrro_el0' use the five system registers. 'elN' is an alias for 'tpidr_elN'.">;
diff --git a/clang/lib/Driver/ToolChains/Arch/ARM.cpp b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
index 3aee540d501be..3567089f53479 100644
--- a/clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -223,6 +223,7 @@ arm::ReadTPMode arm::getReadTPMode(const Driver &D, const ArgList &Args,
             .Case("tpidruro", ReadTPMode::TPIDRURO)
             .Case("tpidrprw", ReadTPMode::TPIDRPRW)
             .Case("soft", ReadTPMode::Soft)
+            .Case("auto", ReadTPMode::Auto)
             .Default(ReadTPMode::Invalid);
     if ((ThreadPointer == ReadTPMode::TPIDRURW ||
          ThreadPointer == ReadTPMode::TPIDRURO ||
@@ -239,7 +240,7 @@ arm::ReadTPMode arm::getReadTPMode(const Driver &D, const ArgList &Args,
       D.Diag(diag::err_drv_invalid_mtp) << A->getAsString(Args);
     return ReadTPMode::Invalid;
   }
-  return ReadTPMode::Soft;
+  return ReadTPMode::Auto;
 }
 
 void arm::setArchNameInTriple(const Driver &D, const ArgList &Args,
@@ -580,6 +581,9 @@ llvm::ARM::FPUKind arm::getARMTargetFeatures(const Driver &D,
     Features.push_back("+read-tp-tpidruro");
   if (getReadTPMode(D, Args, Triple, ForAS) == ReadTPMode::TPIDRPRW)
     Features.push_back("+read-tp-tpidrprw");
+  if (getReadTPMode(D, Args, Triple, ForAS) == ReadTPMode::Auto
+      && isHardTPSupported(Triple) && !ForAS)
+    Features.push_back("+read-tp-tpidrprw");
 
   const Arg *ArchArg = Args.getLastArg(options::OPT_march_EQ);
   const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);
diff --git a/clang/lib/Driver/ToolChains/Arch/ARM.h b/clang/lib/Driver/ToolChains/Arch/ARM.h
index a23a8793a89e2..622383cf0025d 100644
--- a/clang/lib/Driver/ToolChains/Arch/ARM.h
+++ b/clang/lib/Driver/ToolChains/Arch/ARM.h
@@ -41,6 +41,7 @@ enum class ReadTPMode {
   TPIDRURW,
   TPIDRURO,
   TPIDRPRW,
+  Auto,
 };
 
 enum class FloatABI {
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 7c50970068fa9..0f2c476686014 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -3728,7 +3728,7 @@ static void RenderSSPOptions(const Driver &D, const ToolChain &TC,
       // Check whether the user asked for something other than -mtp=cp15
       if (Arg *A = Args.getLastArg(options::OPT_mtp_mode_EQ)) {
         StringRef Value = A->getValue();
-        if (Value != "cp15") {
+        if (Value != "cp15" && Value != "auto") {
           D.Diag(diag::err_drv_argument_not_allowed_with)
               << A->getAsString(Args) << "-mstack-protector-guard=tls";
           return;
diff --git a/clang/test/Driver/arm-thread-pointer.c b/clang/test/Driver/arm-thread-pointer.c
index 5521e1865b276..985c5046f6d26 100644
--- a/clang/test/Driver/arm-thread-pointer.c
+++ b/clang/test/Driver/arm-thread-pointer.c
@@ -42,4 +42,8 @@
 
 // RUN: %clang --target=armv7-linux -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER_NON %s
-// ARMv7_THREAD_POINTER_NON-NOT: "-target-feature" "+read-tp-tpidruro"
+// ARMv7_THREAD_POINTER_NON: "-target-feature" "+read-tp-tpidruro"
+
+// RUN: %clang --target=armv7-linux -mtp=auto -### -S %s 2>&1 | \
+// RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER_Auto %s
+// ARMv7_THREAD_POINTER_Auto: "-target-feature" "+read-tp-tpidruro"

github-actions · 2025-02-25T15:36:36Z

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

statham-arm

Would it be possible to modify the description of this PR so that it generates a commit message that explains the change on its own, without having to refer to a Github ticket containing a discussion?

If I were writing this change, I'd write a commit message something like the following. (You're welcome to use this template if you like, although the [FIXME]s should be edited to fill in the actual facts.)

[ARM] Introduce -mtp=auto and make it the default

This adds a new value auto to the possible values of the existing -mtp= clang option which controls how the thread pointer is found. auto means the same as soft if the target architecture doesn't support a hardware thread pointer at all; otherwise it means the same as [FIXME].

This behavior is the default in gcc version [FIXME] and later. The new auto option is therefore also the default in clang, so this change aligns clang with gcc.

statham-arm · 2025-02-26T09:53:26Z

clang/test/Driver/arm-thread-pointer.c

@@ -42,4 +42,8 @@

 // RUN: %clang --target=armv7-linux -### -S %s 2>&1 | \
 // RUN: FileCheck -check-prefix=ARMv7_THREAD_POINTER_NON %s
-// ARMv7_THREAD_POINTER_NON-NOT: "-target-feature" "+read-tp-tpidruro"
+// ARMv7_THREAD_POINTER_NON: "-target-feature" "+read-tp-tpidruro"


I don't understand how this test matches the change in getARMTargetFeatures. In that function, you respond to -mtp=auto by setting the feature +read-tp-tpidrprw. But here the test is expecting +read-tp-tpidruro, which isn't the same. Can you explain?

It‘s my fault, I want to set +read-tp-tpidruro by -mtp=auto

smithp35

As statham-arm points out, please do replace the [FIXME]s in the description. The first one is tpidruro, you can probably find the GCC version from the changelog.

smithp35 · 2025-02-26T10:54:04Z

clang/lib/Driver/ToolChains/Arch/ARM.cpp

@@ -580,6 +581,9 @@ llvm::ARM::FPUKind arm::getARMTargetFeatures(const Driver &D,
    Features.push_back("+read-tp-tpidruro");
  if (getReadTPMode(D, Args, Triple, ForAS) == ReadTPMode::TPIDRPRW)
    Features.push_back("+read-tp-tpidrprw");
+  if (getReadTPMode(D, Args, Triple, ForAS) == ReadTPMode::Auto
+      && isHardTPSupported(Triple) && !ForAS)
+    Features.push_back("+read-tp-tpidrprw");


I think you mean +read-tp-tpidruro here as that is what GCC defaults to.

smithp35 · 2025-02-26T11:10:34Z

clang/lib/Driver/ToolChains/Arch/ARM.cpp

@@ -580,6 +581,9 @@ llvm::ARM::FPUKind arm::getARMTargetFeatures(const Driver &D,
    Features.push_back("+read-tp-tpidruro");


We're calling getReadTPMode 4 times now, with the same parameters. and I don't think it can return a different value each time. I think this could be simplified to

arm::ReadTPMode TPMode = getReadTPMode(D, Args, Triple, ForAS); if (TPMode == ReadTPMode::TPIDRURW) Features.push_back("+read-tp-tpidrurw"); else if (TPMode == ReadTPMode::TPIDRURO) ...

smithp35 · 2025-02-26T11:23:42Z

clang/lib/Driver/ToolChains/Arch/ARM.cpp

@@ -239,7 +240,7 @@ arm::ReadTPMode arm::getReadTPMode(const Driver &D, const ArgList &Args,
      D.Diag(diag::err_drv_invalid_mtp) << A->getAsString(Args);
    return ReadTPMode::Invalid;
  }
-  return ReadTPMode::Soft;
+  return ReadTPMode::Auto;


Is there a way of rewriting this so that we don't have to return Auto from the function? I think the intention is that the logic of handling the mtp option is centralised here. Ideally we should translate Auto to either Soft or TPIDRURO. That would mean it wouldn't need to be handled in setArchNameInTriple

This may need some reorganisation to avoid duplication. Although it may just be simple enough to live with it.

if (ThreadPointer == ReadTPMode::Auto) return (isHardTPSupported(Triple) ? ReadTPMode::TPIDRURO : ReadTPMode::Soft);

At the end we could replace return ReadTPMode::Auto with

return (isHardTPSupported(Triple) ? ReadTPMode::TPIDRURO : ReadTPMode::Soft);

smithp35 · 2025-02-26T13:28:47Z

A couple of things I forgot to mention:

Please can you update the in progress ReleaseNotes in https://github.com/llvm/llvm-project/blob/main/clang/docs/ReleaseNotes.rst as this is a potentially breaking change.
It is fine to add fixes https://github.com/llvm/llvm-project/issues/123864 to the Description so that the issue is automatically closed.

statham-arm · 2025-02-26T13:34:07Z

It is fine to add fixes https://github.com/llvm/llvm-project/issues/123864 to the Description so that the issue is automatically closed.

Yes, good point! When I suggested an improved description, I left out that part, which is the one thing @Zhenhang1213 didn't forget in the original. Sorry.

This adds a new value auto to the possible values of the existing -mtp= clang option which controls how the thread pointer is found. auto means the same as soft if the target architecture doesn't support a hardware thread pointer at all; otherwise it means the same as cp15. This behavior is the default in gcc version 7.3.0 and later. The new auto option is therefore also the default in clang, so this change aligns clang with gcc.

llvmbot added clang Clang issues not falling into any other category backend:ARM clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Feb 25, 2025

Zhenhang1213 force-pushed the main branch from b021716 to 4ca0fa7 Compare February 25, 2025 15:28

CoTinker requested a review from statham-arm February 26, 2025 01:18

davemgreen requested review from ostannard and smithp35 February 26, 2025 08:17

statham-arm reviewed Feb 26, 2025

View reviewed changes

Zhenhang1213 changed the title ~~[ARM] Aligned mtp behavior and gcc~~ [ARM] Introduce -mtp=auto and make it the default Feb 26, 2025

smithp35 reviewed Feb 26, 2025

View reviewed changes

Zhenhang1213 force-pushed the main branch from e3cf180 to 7a1c99a Compare February 26, 2025 15:19

Zhenhang1213 force-pushed the main branch from 7a1c99a to 019e943 Compare February 26, 2025 15:23

Zhenhang1213 and others added 5 commits February 26, 2025 23:25

Merge branch 'main' into main

1fa8e71

Merge branch 'llvm:main' into main

4cd6ad8

Merge branch 'llvm:main' into main

ada289d

Merge branch 'main' of https://github.com/Zhenhang1213/llvm-project

bd0307b

Zhenhang1213 closed this Feb 26, 2025

smithp35 mentioned this pull request Feb 27, 2025

[ARM] Introduce -mtp=auto and make it the default #128901

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ARM] Introduce -mtp=auto and make it the default #128728

[ARM] Introduce -mtp=auto and make it the default #128728

Zhenhang1213 commented Feb 25, 2025 •

edited

Loading

llvmbot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading

statham-arm left a comment

statham-arm Feb 26, 2025

Zhenhang1213 Feb 26, 2025

smithp35 left a comment

smithp35 Feb 26, 2025

smithp35 Feb 26, 2025

smithp35 Feb 26, 2025

smithp35 commented Feb 26, 2025

statham-arm commented Feb 26, 2025

		@@ -580,6 +581,9 @@ llvm::ARM::FPUKind arm::getARMTargetFeatures(const Driver &D,
		Features.push_back("+read-tp-tpidruro");

[ARM] Introduce -mtp=auto and make it the default #128728

[ARM] Introduce -mtp=auto and make it the default #128728

Conversation

Zhenhang1213 commented Feb 25, 2025 • edited Loading

llvmbot commented Feb 25, 2025 • edited Loading

github-actions bot commented Feb 25, 2025 • edited Loading

statham-arm left a comment

Choose a reason for hiding this comment

statham-arm Feb 26, 2025

Choose a reason for hiding this comment

Zhenhang1213 Feb 26, 2025

Choose a reason for hiding this comment

smithp35 left a comment

Choose a reason for hiding this comment

smithp35 Feb 26, 2025

Choose a reason for hiding this comment

smithp35 Feb 26, 2025

Choose a reason for hiding this comment

smithp35 Feb 26, 2025

Choose a reason for hiding this comment

smithp35 commented Feb 26, 2025

statham-arm commented Feb 26, 2025

Zhenhang1213 commented Feb 25, 2025 •

edited

Loading

llvmbot commented Feb 25, 2025 •

edited

Loading

github-actions bot commented Feb 25, 2025 •

edited

Loading