[PATCH] D156203: [AIX][TLS] Add target attribute for -maix-small-local-exec-tls option.

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPC.td:323-327
+                   "Produce a faster access sequence for local-exec TLS "
+                   "variables where the offset from the TLS base "
+                   "is encoded as an immediate operand (AIX 64-bit only). "
+                   "This access sequence is not used for variables larger "
+                   "than 32KB.">;
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > nemanjai wrote:
> > > I don't think it's useful to add the entire help text here. I think something like:
> > > ```
> > > Produce TOC-free TLS sequence for this function for 64-bit AIX
> > > ```
> > > And describe in the comment that this is not really a Subtarget feature, but it it is used for convenience.
> > > ```
> > > // Specifies that TLS access in any function with this target attribute should
> > > // use the optimized TOC-free sequence (where the offset is an immediate
> > > // off of R13 for which the linker might add fix-up code if the immediate
> > > // is too large). Clearly, this isn't really a feature of the subtarget, but is
> > > // used as a convenient way to affect code generation for individual functions.
> > > ```
> > > 
> > > Also, I don't think we need to use such a verbose function here. This isn't really a user-facing option, so I think we can safely abbreviate it a bit. Maybe `aix-smalltls`.
> > `aix-smalltls` is ambiguous with potential future development of the analogous local-dynamic access sequence.
> > ```
> > Produce TOC-free TLS sequence for this function for 64-bit AIX
> > ```
> 
> Similarly, this is only true for local-exec cases.
I've updated the comment a bit to address both Nemanja and Hubert's concerns.
As Hubert mentioned, I will probably just keep the option as `aix-small-local-exec-tls`.


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.cpp:128-135
+    if (!TargetTriple.isOSAIX())
+      report_fatal_error(
+        "The aix-small-local-exec-tls attribute is only supported on AIX.\n",
+        false);
+    if (!IsPPC64)
+      report_fatal_error(
+        "The aix-small-local-exec-tls attribute is currently only supported "
----------------
nemanjai wrote:
> I think it suffices to just fold this into a single condition:
> ```
> if (HasAIXSmallLocalExecTLS && (!TargetTriple.isOSAIX() || !IsPPC64))
>   report_fatal_error(
>     "The aix-small-local-tls attribute is only supported on AIX in 64-bit mode");
> ```
Yeah, I agree.
Done.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156203