[PATCH] D155600: [AIX][TLS] Produce a faster local-exec access sequence with -maix-small-local-exec-tls (And optimize when load/store offsets are 0)

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added a comment.

Additional partial re-review comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:147-149
+// A faster local-exec TLS access sequence (enabled with the
+// -maix-small-local-exec-tls option) can be produced for TLS variables with a
+// max size of slightly under 32KB.
----------------
We chose the maximum based on the behaviour of the reference implementation (not from first principles).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:150
+// max size of slightly under 32KB.
+constexpr uint64_t AIXTLSUpperDisplacement = 32752;
+
----------------
"Displacement" implies "offset", not size. Maybe `AIXSmallTlsPolicySizeLimit`?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3349-3361
+      // Produce a faster access sequence for local-exec TLS variables where
+      // the offset from the TLS base is encoded as an immediate operand.
+      //
+      // In order to utilize a faster local-exec access sequence (enabled by the
+      // -maix-small-local-exec-tls option), the size of the TLS variable that
+      // is being considered for the faster sequence needs to be roughly
+      // under 32KB (represented by AIXTLSUpperDisplacement). The size needs
----------------
Since the limit is not derived from first principles, phrase as a policy choice. Additionally, start off by mentioning the option (so the comments make sense in the context of the earlier comment about the TOC-based path).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3362-3366
+      Type *GVType = GV->getValueType();
+      if (GVType->isSized() && !GVType->isEmptyTy()) {
+        uint64_t GVTypeSize =
+            GV->getParent()->getDataLayout().getTypeAllocSize(GVType);
+        if (HasAIXSmallLocalExecTLS && (GVTypeSize < AIXTLSUpperDisplacement))
----------------
Check `HasAIXSmallLocalExecTLS` first before setting up for more checks.


================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:115-116
     RefKind = MCSymbolRefExpr::VK_PPC_GOT_TPREL_PCREL;
+  else if (MO.getTargetFlags() == PPCII::MO_TPREL_FLAG)
+    RefKind = MCSymbolRefExpr::VK_PPC_AIX_TLSLE;
 
----------------
amyk wrote:
> hubert.reinterpretcast wrote:
> > In light of https://reviews.llvm.org/D156292, I believe this should have an assertion added about the TLS model (see `llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp` change in the aforementioned patch).
> I have updated it to add an assert for the local-exec model. Is this what you meant?
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600