[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

In D155544#4523857 , @amyk wrote:

> Wouldn't this patch need to land before the back-end patch, because I 
> introduce the option here, and then I use it in the backend patch?

Please move the `llvm/` changes into the back-end patch or land them separately 
first.

> In terms of checking for a 32-bit diagnostic within 
> `check-aix-small-local-exec-tls-opt.ll`, the RUN lines in that test currently 
> are 64-bit RUN lines. Perhaps in the backend patch, I should just add the 
> 32-bit lines to show the diagnostic?

This seems to be the test designed to cover the non-codegen aspects of the 
attribute. In that light, the 32-bit run line belongs here.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155544/new/

https://reviews.llvm.org/D155544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Amy Kwan via Phabricator via cfe-commits]

amyk added a comment.

In D155544#4523536 , 
@hubert.reinterpretcast wrote:

> Patch should not land before back-end patch. I also suggest having the patch 
> incorporate the new option into the Clang release notes before it lands.

I'm currently addressing reviews for the back-end patch but just wanted to 
clarify because I might be misunderstanding something.

Wouldn't this patch need to land before the back-end patch, because I introduce 
the option here, and then I use it in the backend patch?
In terms of checking for a 32-bit diagnostic within 
`check-aix-small-local-exec-tls-opt.ll`, the RUN lines in that test currently 
are 64-bit RUN lines. Perhaps in the backend patch, I should just add the 
32-bit lines to show the diagnostic?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155544/new/

https://reviews.llvm.org/D155544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4095
+  HelpText<"Produce a faster access sequence for local-exec TLS variables "
+   "where the offset from the thread pointer value is encoded as an "
+   "immediate operand (AIX 64-bit only). "

Same comment: use "TLS base".



Comment at: clang/test/Driver/aix-small-local-exec-tls.c:2
+// RUN: %clang -target powerpc64-unknown-aix -S -emit-llvm %s -o - | FileCheck 
%s
+// RUN: %clang -target powerpc-unknown-aix -S -emit-llvm %s -o - | FileCheck %s
+// RUN: %clang -target powerpc64le-unknown-linux-gnu -S -emit-llvm %s -o - | 
FileCheck %s

It was agreed in offline discussion that 32-bit AIX should generate a message 
about the option because the general semantics are meaningful in that context 
but there is no implementation.



Comment at: llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll:6
+; RUN:   FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
+
+define dso_local signext i32 @f() {

Same comment re: 32-bit AIX.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155544/new/

https://reviews.llvm.org/D155544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

Patch should not land before back-end patch. I also suggest having the patch 
incorporate the new option into the Clang release notes before it lands.




Comment at: llvm/lib/Target/PowerPC/PPC.td:324
+   "Produce a faster access sequence for local-exec TLS "
+   "variables where the offset from the thread pointer value "
+   "is encoded as an immediate operand (AIX 64-bit only). "

As agreed offline:
thread pointer value -> TLS base




Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.cpp:127
+
+  if ((!TargetTriple.isOSAIX()) && HasAIXSmallLocalExecTLS)
+report_fatal_error(

There should be no confusion over how tightly `!` binds over `&&`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155544/new/

https://reviews.llvm.org/D155544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Stefan Pintilie via Phabricator via cfe-commits]

stefanp accepted this revision as: stefanp.
stefanp added a comment.
This revision is now accepted and ready to land.

I think this patch makes sense to me.
LGTM.




Comment at: llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll:15
+
+; Make sure that the test was actually compiled successfully after using the
+; -maix-small-local-exec-tls option.

DiggerLin wrote:
> since the patch only add a new option "aix-small-local-exec-tls" , the the 
> backend implementation for this option in not in the patch, the behavior  of 
> the CodeGen do not change in the patch, I do not think we need the test case. 
I would actually prefer to have a test case here. We do need to check to make 
sure that the backend produces the correct error on Linux and produces valid 
codegen on AIX. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155544/new/

https://reviews.llvm.org/D155544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Digger Lin via Phabricator via cfe-commits]

DiggerLin added inline comments.



Comment at: llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll:15
+
+; Make sure that the test was actually compiled successfully after using the
+; -maix-small-local-exec-tls option.

since the patch only add a new option "aix-small-local-exec-tls" , the the 
backend implementation for this option in not in the patch, the behavior  of 
the CodeGen do not change in the patch, I do not think we need the test case. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155544/new/

https://reviews.llvm.org/D155544

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D155544: [AIX][TLS] Add -maix-small-local-exec-tls option.

[Amy Kwan via Phabricator via cfe-commits]

amyk created this revision.
amyk added reviewers: PowerPC, stefanp, kamaub, nemanjai, 
hubert.reinterpretcast.
amyk added a project: LLVM.
Herald added subscribers: kbarton, hiraditya.
Herald added a project: All.
amyk requested review of this revision.
Herald added a project: clang.
Herald added subscribers: llvm-commits, cfe-commits.

This patch adds an AIX-specific option to inform the compiler that it can use
a faster access sequence for the local-exec TLS model (formally named
`aix-small-local-exec-tls`).

This patch only adds the option, and the backend implementation for this option
will be added in a follow up patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D155544

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Basic/Targets/PPC.cpp
  clang/lib/Basic/Targets/PPC.h
  clang/test/Driver/aix-small-local-exec-tls.c
  clang/test/OpenMP/target_data_map_codegen_hold.cpp
  llvm/lib/Target/PowerPC/PPC.td
  llvm/lib/Target/PowerPC/PPCSubtarget.cpp
  llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll

Index: llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll
===
--- /dev/null
+++ llvm/test/CodeGen/PowerPC/check-aix-small-local-exec-tls-opt.ll
@@ -0,0 +1,18 @@
+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff -mattr=+aix-small-local-exec-tls \
+; RUN:   -ppc-asm-full-reg-names < %s | FileCheck %s
+; RUN: not llc -mtriple powerpc64le-unknown-linux-gnu -mattr=+aix-small-local-exec-tls \
+; RUN:   -ppc-asm-full-reg-names < %s 2>&1 | \
+; RUN:   FileCheck %s --check-prefix=CHECK-NOT-SUPPORTED
+
+define dso_local signext i32 @f() {
+entry:
+  ret i32 0
+}
+
+; Check that the -maix-small-local-exec-tls option is not supported on Linux.
+; CHECK-NOT-SUPPORTED: The aix-small-local-exec-tls attribute is only supported on AIX.
+
+; Make sure that the test was actually compiled successfully after using the
+; -maix-small-local-exec-tls option.
+; CHECK:li r3, 0
+; CHECK-NEXT:   blr
Index: llvm/lib/Target/PowerPC/PPCSubtarget.cpp
===
--- llvm/lib/Target/PowerPC/PPCSubtarget.cpp
+++ llvm/lib/Target/PowerPC/PPCSubtarget.cpp
@@ -123,6 +123,11 @@
 
   // Determine endianness.
   IsLittleEndian = TM.isLittleEndian();
+
+  if ((!TargetTriple.isOSAIX()) && HasAIXSmallLocalExecTLS)
+report_fatal_error(
+  "The aix-small-local-exec-tls attribute is only supported on AIX.\n",
+  false);
 }
 
 bool PPCSubtarget::enableMachineScheduler() const { return true; }
Index: llvm/lib/Target/PowerPC/PPC.td
===
--- llvm/lib/Target/PowerPC/PPC.td
+++ llvm/lib/Target/PowerPC/PPC.td
@@ -318,6 +318,14 @@
   SubtargetFeature<"privileged", "HasPrivileged", "true",
"Add privileged instructions">;
 
+def FeatureAIXLocalExecTLS :
+  SubtargetFeature<"aix-small-local-exec-tls", "HasAIXSmallLocalExecTLS", "true",
+   "Produce a faster access sequence for local-exec TLS "
+   "variables where the offset from the thread pointer value "
+   "is encoded as an immediate operand (AIX 64-bit only). "
+   "This access sequence is not used for variables larger "
+   "than 32KB.">;
+
 def FeaturePredictableSelectIsExpensive :
   SubtargetFeature<"predictable-select-expensive",
"PredictableSelectIsExpensive",
Index: clang/test/OpenMP/target_data_map_codegen_hold.cpp
===
--- clang/test/OpenMP/target_data_map_codegen_hold.cpp
+++ clang/test/OpenMP/target_data_map_codegen_hold.cpp
@@ -517,7 +517,7 @@
 
 #endif
 //.
-// CHECK-PPC64LE: attributes #[[ATTR0:[0-9]+]] = { mustprogress noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="-altivec,-bpermd,-crbits,-crypto,-direct-move,-extdiv,-htm,-isa-v206-instructions,-isa-v207-instructions,-isa-v30-instructions,-power8-vector,-power9-vector,-privileged,-quadword-atomics,-rop-protect,-spe,-vsx" }
+// CHECK-PPC64LE: attributes #[[ATTR0:[0-9]+]] = { mustprogress noinline nounwind optnone "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-features"="-aix-small-local-exec-tls,-altivec,-bpermd,-crbits,-crypto,-direct-move,-extdiv,-htm,-isa-v206-instructions,-isa-v207-instructions,-isa-v30-instructions,-power8-vector,-power9-vector,-privileged,-quadword-atomics,-rop-protect,-spe,-vsx" }
 // CHECK-PPC64LE: attributes #[[ATTR1:[0-9]+]] = { nounwind }
 // CHECK-PPC64LE: attributes #[[ATTR2:[0-9]+]] = { nocallback nofree nounwind willreturn memory(argmem: readwrite) }
 //.
Index: clang/test/Driver/aix-small-local-exec-tls.c
===
--- /dev/null
+++ clang/test/Driver/aix-small-local-exec-tls.c
@@ -0,0 +1,29 @@
+// RUN: %clang -target powerpc64-unknown-aix -S