Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][driver] Set TLSDESC as the default for Android on RISC-V #81198

Conversation

ilovepi
Copy link
Contributor

@ilovepi ilovepi commented Feb 8, 2024

No description provided.

Created using spr 1.3.4
@pirama-arumuga-nainar
Copy link
Collaborator

Can you add a test similar to clang/test/Driver/emulated-tls.cpp?

@ilovepi
Copy link
Contributor Author

ilovepi commented Feb 8, 2024

Can you add a test similar to clang/test/Driver/emulated-tls.cpp?

I knew I was forgetting something when I threw this up. Thank you!

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Feb 9, 2024
@llvmbot
Copy link
Member

llvmbot commented Feb 9, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Paul Kirth (ilovepi)

Changes

Sets TLSDESC as the default on Android versions newer than 29, or if its
Android on RISC-V.


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

2 Files Affected:

  • (modified) clang/test/Driver/tls-dialect.c (+5)
  • (modified) llvm/include/llvm/TargetParser/Triple.h (+1-2)
diff --git a/clang/test/Driver/tls-dialect.c b/clang/test/Driver/tls-dialect.c
index 4e105ce3cea5d9..f309f4a44fbc3f 100644
--- a/clang/test/Driver/tls-dialect.c
+++ b/clang/test/Driver/tls-dialect.c
@@ -3,6 +3,11 @@
 // RUN: %clang -### --target=riscv64-linux %s 2>&1 | FileCheck --check-prefix=NODESC %s
 // RUN: %clang -### --target=x86_64-linux -mtls-dialect=gnu %s 2>&1 | FileCheck --check-prefix=NODESC %s
 
+/// Android supports TLSDESC by default after Android version 29 and all RISC-V
+/// TLSDESC is not on by default in Linux, even on RISC-V
+// RUN: %clang -### --target=riscv64-android %s 2>&1 | FileCheck --check-prefix=DESC %s
+// RUN: %clang -### --target=riscv64-linux %s 2>&1 | FileCheck --check-prefix=NODESC %s
+
 /// LTO
 // RUN: %clang -### --target=riscv64-linux -flto -mtls-dialect=desc %s 2>&1 | FileCheck --check-prefix=LTO-DESC %s
 // RUN: %clang -### --target=riscv64-linux -flto %s 2>&1 | FileCheck --check-prefix=LTO-NODESC %s
diff --git a/llvm/include/llvm/TargetParser/Triple.h b/llvm/include/llvm/TargetParser/Triple.h
index 98d8490cc9f7a2..0382d97d3fe38b 100644
--- a/llvm/include/llvm/TargetParser/Triple.h
+++ b/llvm/include/llvm/TargetParser/Triple.h
@@ -1036,8 +1036,7 @@ class Triple {
   /// True if the target supports both general-dynamic and TLSDESC, and TLSDESC
   /// is enabled by default.
   bool hasDefaultTLSDESC() const {
-    // TODO: Improve check for other platforms, like Android, and RISC-V
-    return false;
+    return isAndroid() && (!isAndroidVersionLT(29) || isRISCV64());
   }
 
   /// Tests whether the target uses -data-sections as default.

@pirama-arumuga-nainar
Copy link
Collaborator

LGTM but let's wait for any comments from enh and maskray as well.

@enh-google
Copy link
Contributor

i think my suggestion would be instead of

    return isAndroid() && (!isAndroidVersionLT(29) || isRISCV64());

just go with

    return isAndroid() && isRISCV64();

that solves today's problem, and we can worry about arm64 later[1]. (and x86-64 if/when that's implemented. and 32-bit never.)


  1. why, when arm64 tlsdesc already shipped, and rv64 hasn't shipped yet? for exactly that reason :-) with rv64 i don't even have to [have rprichard] think about app compat issues, say, or [have danalbert] think about communicating the change to ndk users, etc...

@ilovepi
Copy link
Contributor Author

ilovepi commented Feb 9, 2024

Sounds good. I’ll update this when I have a chance tomorrow.

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once the !isAndroidVersionLT(29) check is removed.

/// Android supports TLSDESC by default after Android version 29 and all RISC-V
/// TLSDESC is not on by default in Linux, even on RISC-V
// RUN: %clang -### --target=riscv64-android %s 2>&1 | FileCheck --check-prefix=DESC %s
// RUN: %clang -### --target=riscv64-linux %s 2>&1 | FileCheck --check-prefix=NODESC %s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete riscv64-linux check. It's redundant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good point. I went ahead and updated the comment to point that out.

@ilovepi ilovepi merged commit 407f9c0 into main Feb 9, 2024
3 of 4 checks passed
@ilovepi ilovepi deleted the users/ilovepi/spr/clangdriver-set-tlsdesc-as-the-default-for-android-on-risc-v branch February 9, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants