[AIX][TLS] Disallow the use of -maix-small-local-exec-tls and -fno-data-sections #79252

amy-kwan · 2024-01-24T04:21:48Z

This patch disallows the use of the -maix-small-local-exec-tls and -fno-data-sections options within clang, and also disallows the use of the aix-small-local-exec-tls attribute with the -data-sections=false option in llc.

This is because having data sections off when using the aix-small-local-exec-tls feature is not ideal for performance. As the small-local-exec-tls region is a limited resource, this space should not used for variables that may be replaced.

Note, that on AIX, data sections is turned on by default, so this patch makes it so that a diagnostic is emitted when users explicitly turn off data sections while using the aix-small-local-exec-tls feature.

llvmbot · 2024-01-24T04:22:18Z

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Amy Kwan (amy-kwan)

Changes

This patch disallows the use of the -maix-small-local-exec-tls and -fno-data-sections options within clang, and also disallows the use of the aix-small-local-exec-tls attribute with the -data-sections=false option in llc.

This is because having data sections off when using the aix-small-local-exec-tls feature is not ideal for performance. As the small-local-exec-tls region is a limited resource, this space should not used for variables that may be replaced.

Note, that on AIX, data sections is turned on by default, so this patch makes it so that a diagnostic is emitted when users explicitly turn off data sections while using the aix-small-local-exec-tls feature.

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

5 Files Affected:

(modified) clang/lib/Basic/Targets/PPC.cpp (-8)
(modified) clang/lib/Driver/ToolChains/Arch/PPC.cpp (+19)
(modified) clang/test/Driver/aix-small-local-exec-tls.c (+7)
(modified) llvm/lib/Target/PowerPC/PPCSubtarget.cpp (+17-4)
(modified) llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll (+7)

diff --git a/clang/lib/Basic/Targets/PPC.cpp b/clang/lib/Basic/Targets/PPC.cpp
index 41935abfb65d3b9..911b80c98f7ad62 100644
--- a/clang/lib/Basic/Targets/PPC.cpp
+++ b/clang/lib/Basic/Targets/PPC.cpp
@@ -646,14 +646,6 @@ bool PPCTargetInfo::initFeatureMap(
     return false;
   }
 
-  if (llvm::is_contained(FeaturesVec, "+aix-small-local-exec-tls")) {
-    if (!getTriple().isOSAIX() || !getTriple().isArch64Bit()) {
-      Diags.Report(diag::err_opt_not_valid_on_target)
-         << "-maix-small-local-exec-tls";
-      return false;
-    }
-  }
-
   return TargetInfo::initFeatureMap(Features, Diags, CPU, FeaturesVec);
 }
 
diff --git a/clang/lib/Driver/ToolChains/Arch/PPC.cpp b/clang/lib/Driver/ToolChains/Arch/PPC.cpp
index ab24d14992cd7a1..5ffe73236205d3b 100644
--- a/clang/lib/Driver/ToolChains/Arch/PPC.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/PPC.cpp
@@ -122,6 +122,25 @@ void ppc::getPPCTargetFeatures(const Driver &D, const llvm::Triple &Triple,
   ppc::ReadGOTPtrMode ReadGOT = ppc::getPPCReadGOTPtrMode(D, Triple, Args);
   if (ReadGOT == ppc::ReadGOTPtrMode::SecurePlt)
     Features.push_back("+secure-plt");
+
+  bool UseSeparateSections = isUseSeparateSections(Triple);
+  bool HasDefaultDataSections = Triple.isOSBinFormatXCOFF();
+  if (Args.hasArg(options::OPT_maix_small_local_exec_tls)) {
+    if (!Triple.isOSAIX() || !Triple.isArch64Bit())
+      D.Diag(diag::err_opt_not_valid_on_target) << "-maix-small-local-exec-tls";
+
+    // The -maix-small-local-exec-tls option should only be used with
+    // -fdata-sections, as having data sections turned off with this option
+    // is not ideal for performance. Moreover, the small-local-exec-tls region
+    // is a limited resource, and should not be used for variables that may
+    // be replaced.
+    if (!Args.hasFlag(options::OPT_fdata_sections,
+                      options::OPT_fno_data_sections,
+                      UseSeparateSections || HasDefaultDataSections))
+      D.Diag(diag::err_drv_argument_only_allowed_with)
+          << "-maix-small-local-exec-tls"
+          << "-fdata-sections";
+  }
 }
 
 ppc::ReadGOTPtrMode ppc::getPPCReadGOTPtrMode(const Driver &D, const llvm::Triple &Triple,
diff --git a/clang/test/Driver/aix-small-local-exec-tls.c b/clang/test/Driver/aix-small-local-exec-tls.c
index 7a2eec6989eef89..e6719502a3babc3 100644
--- a/clang/test/Driver/aix-small-local-exec-tls.c
+++ b/clang/test/Driver/aix-small-local-exec-tls.c
@@ -12,6 +12,12 @@
 // RUN:    -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-UNSUPPORTED-LINUX %s
 // RUN: not %clang -target powerpc64-unknown-linux-gnu -maix-small-local-exec-tls \
 // RUN:    -fsyntax-only %s 2>&1 | FileCheck --check-prefix=CHECK-UNSUPPORTED-LINUX %s
+// RUN: not %clang -target powerpc64-unknown-aix -maix-small-local-exec-tls \
+// RUN:    -fsyntax-only -fno-data-sections %s 2>&1 | \
+// RUN:    FileCheck --check-prefix=CHECK-UNSUPPORTED-NO-DATASEC %s
+// RUN: not %clang -target powerpc64-unknown-linux-gnu -maix-small-local-exec-tls \
+// RUN:    -fsyntax-only -fno-data-sections %s 2>&1 | \
+// RUN:    FileCheck --check-prefix=CHECK-UNSUPPORTED-NO-DATASEC %s
 
 int test(void) {
   return 0;
@@ -23,6 +29,7 @@ int test(void) {
 
 // CHECK-UNSUPPORTED-AIX32: option '-maix-small-local-exec-tls' cannot be specified on this target
 // CHECK-UNSUPPORTED-LINUX: option '-maix-small-local-exec-tls' cannot be specified on this target
+// CHECK-UNSUPPORTED-NO-DATASEC: invalid argument '-maix-small-local-exec-tls' only allowed with '-fdata-sections'
 
 // CHECK-AIX_SMALL_LOCALEXEC_TLS: test() #0 {
 // CHECK-AIX_SMALL_LOCALEXEC_TLS: attributes #0 = {
diff --git a/llvm/lib/Target/PowerPC/PPCSubtarget.cpp b/llvm/lib/Target/PowerPC/PPCSubtarget.cpp
index c9740818c9bfd6f..2735bdee3bcfcc1 100644
--- a/llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ b/llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -124,10 +124,23 @@ void PPCSubtarget::initSubtargetFeatures(StringRef CPU, StringRef TuneCPU,
   // Determine endianness.
   IsLittleEndian = TM.isLittleEndian();
 
-  if (HasAIXSmallLocalExecTLS && (!TargetTriple.isOSAIX() || !IsPPC64))
-    report_fatal_error(
-      "The aix-small-local-exec-tls attribute is only supported on AIX in "
-      "64-bit mode.\n", false);
+  if (HasAIXSmallLocalExecTLS) {
+    if (!TargetTriple.isOSAIX() || !IsPPC64)
+      report_fatal_error(
+          "The aix-small-local-exec-tls attribute is only supported on AIX in "
+          "64-bit mode.\n",
+          false);
+    // The aix-small-local-exec-tls attribute should only be used with
+    // -data-sections, as having data sections turned off with this option
+    // is not ideal for performance. Moreover, the small-local-exec-tls region
+    // is a limited resource, and should not be used for variables that may
+    // be replaced.
+    if (!TM.getDataSections())
+      report_fatal_error(
+          "The aix-small-local-exec-tls attribute can only be specified with "
+          "-data-sections.\n",
+          false);
+  }
 }
 
 bool PPCSubtarget::enableMachineScheduler() const { return true; }
diff --git a/llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll b/llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll
index 62ec0974b0eb55d..00f58ecda15fe1f 100644
--- a/llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll
+++ b/llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll
@@ -4,6 +4,9 @@
 ; RUN:   < %s 2>&1 | FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
 ; RUN: not llc -mtriple powerpc64le-unknown-linux-gnu -ppc-asm-full-reg-names \
 ; RUN:   < %s 2>&1 | FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
+; RUN: not llc -mtriple powerpc64-ibm-aix-xcoff -ppc-asm-full-reg-names \
+; RUN:   -data-sections=false < %s 2>&1 | \
+; RUN: FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED-NO-DATASEC
 
 define dso_local signext i32 @testWithIRAttr() #0 {
 entry:
@@ -12,6 +15,10 @@ entry:
 ; Check that the aix-small-local-exec-tls attribute is not supported on Linux and AIX (32-bit).
 ; CHECK-NOT-SUPPORTED: The aix-small-local-exec-tls attribute is only supported on AIX in 64-bit mode.
 
+; Check that the aix-small-local-exec-tls attribute is only supported when
+; data sections are enabled.
+; CHECK-NOT-SUPPORTED-NO-DATASEC: The aix-small-local-exec-tls attribute can only be specified with -data-sections.
+
 ; Make sure that the test was actually compiled successfully after using the
 ; aix-small-local-exec-tls attribute.
 ; CHECK-LABEL: testWithIRAttr:

stefanp-synopsys

LGTM
Only one minor nit.

stefanp-synopsys · 2024-01-25T16:45:07Z

llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll

@@ -4,6 +4,9 @@
 ; RUN:   < %s 2>&1 | FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
 ; RUN: not llc -mtriple powerpc64le-unknown-linux-gnu -ppc-asm-full-reg-names \
 ; RUN:   < %s 2>&1 | FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
+; RUN: not llc -mtriple powerpc64-ibm-aix-xcoff -ppc-asm-full-reg-names \
+; RUN:   -data-sections=false < %s 2>&1 | \
+; RUN: FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED-NO-DATASEC


nit:
I realize that this is how things are done above but using CHECK-NOT-SOMETHING is really confusing. I had to wrap my brain around it and realize that this was indeed a CHECK and not a CHECK-NOT.

How about CHECK-UNSUPPORTED-NO-DATASEC like the first test case.

Sure, that is a good point. Let me update that.

…ta-sections This patch disallows the use of the -maix-small-local-exec-tls and -fno-data-sections options within clang, and also disallows the use of the aix-small-local-exec-tls attribute with the -data-sections=false option in llc. This is because having data sections off when using the aix-small-local-exec-tls feature is not ideal for performance. As the small-local-exec-tls region is a limited resource, this space should not used for variables that may be replaced. Note, that on AIX, data sections is turned on by default, so this patch makes it so that a diagnostic is emitted when users explicitly turn off data sections while using the aix-small-local-exec-tls feature.

…ribute.ll.

stephenpeckham · 2024-01-26T14:06:24Z

llvm/lib/Target/PowerPC/PPCSubtarget.cpp

+    if (!TM.getDataSections())
+      report_fatal_error(
+          "The aix-small-local-exec-tls attribute can only be specified with "
+          "-data-sections.\n",


Suggested change

"-data-sections.\n",

"-fdata-sections.\n",

I kept the llvm portion as -data-sections since the option for llc is -data-sections rather than -fdata-sections (which is the option for clang).

stephenpeckham · 2024-01-26T14:06:37Z

llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt-IRattribute.ll

@@ -12,6 +15,10 @@ entry:
 ; Check that the aix-small-local-exec-tls attribute is not supported on Linux and AIX (32-bit).
 ; CHECK-NOT-SUPPORTED: The aix-small-local-exec-tls attribute is only supported on AIX in 64-bit mode.

+; Check that the aix-small-local-exec-tls attribute is only supported when
+; data sections are enabled.
+; CHECK-UNSUPPORTED-NO-DATASEC: The aix-small-local-exec-tls attribute can only be specified with -data-sections.


Suggested change

; CHECK-UNSUPPORTED-NO-DATASEC: The aix-small-local-exec-tls attribute can only be specified with -data-sections.

; CHECK-UNSUPPORTED-NO-DATASEC: The aix-small-local-exec-tls attribute can only be specified with -fdata-sections.

I kept the llvm portion as -data-sections since the option for llc is -data-sections rather than -fdata-sections (which is the option for clang).

amy-kwan self-assigned this Jan 24, 2024

llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jan 24, 2024

amy-kwan added backend:PowerPC platform:aix labels Jan 24, 2024

amy-kwan requested review from hubert-reinterpretcast, diggerlin, stefanp-synopsys, stephenpeckham, kamaub and lei137 January 24, 2024 04:22

amy-kwan force-pushed the amy-kwan/prevent-smalllocalexectls-nodatasections branch from c6fb2a4 to 4999dba Compare January 24, 2024 14:47

stefanp-synopsys approved these changes Jan 25, 2024

View reviewed changes

amy-kwan force-pushed the amy-kwan/prevent-smalllocalexectls-nodatasections branch 3 times, most recently from a7c6644 to 5d2fa75 Compare January 25, 2024 19:57

amy-kwan added 2 commits January 25, 2024 23:22

Update CHECK prefix name for check-aix-small-local-exec-tls-opt-IRatt…

0771303

…ribute.ll.

amy-kwan force-pushed the amy-kwan/prevent-smalllocalexectls-nodatasections branch from 5d2fa75 to 0771303 Compare January 26, 2024 04:22

stephenpeckham reviewed Jan 26, 2024

View reviewed changes

amy-kwan merged commit d5fe1bd into llvm:main Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[AIX][TLS] Disallow the use of -maix-small-local-exec-tls and -fno-data-sections #79252

[AIX][TLS] Disallow the use of -maix-small-local-exec-tls and -fno-data-sections #79252

Uh oh!

amy-kwan commented Jan 24, 2024

Uh oh!

llvmbot commented Jan 24, 2024 •

edited

Loading

Uh oh!

stefanp-synopsys left a comment

Uh oh!

stefanp-synopsys Jan 25, 2024

Uh oh!

amy-kwan Jan 25, 2024

Uh oh!

stephenpeckham Jan 26, 2024

Uh oh!

amy-kwan Jan 26, 2024

Uh oh!

stephenpeckham Jan 26, 2024

Uh oh!

amy-kwan Jan 26, 2024

Uh oh!

Uh oh!

	; CHECK-UNSUPPORTED-NO-DATASEC: The aix-small-local-exec-tls attribute can only be specified with -data-sections.
	; CHECK-UNSUPPORTED-NO-DATASEC: The aix-small-local-exec-tls attribute can only be specified with -fdata-sections.

[AIX][TLS] Disallow the use of -maix-small-local-exec-tls and -fno-data-sections #79252

[AIX][TLS] Disallow the use of -maix-small-local-exec-tls and -fno-data-sections #79252

Uh oh!

Conversation

amy-kwan commented Jan 24, 2024

Uh oh!

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

Uh oh!

Uh oh!

stefanp-synopsys left a comment

Choose a reason for hiding this comment

Uh oh!

stefanp-synopsys Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

amy-kwan Jan 25, 2024

Choose a reason for hiding this comment

Uh oh!

stephenpeckham Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

amy-kwan Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

stephenpeckham Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

amy-kwan Jan 26, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented Jan 24, 2024 •

edited

Loading