[PATCH] D149722: [AIX][TLS] Generate 64-bit local-exec access code sequence

[Kamau Bridgeman via Phabricator via llvm-commits]

kamaub added a comment.

Thank you for this patch, just some review comments.



================
Comment at: llvm/lib/Target/PowerPC/PPC.h:131
     /// MO_TPREL_FLAG - If this bit is set the symbol reference is relative to
-    /// TLS Initial Exec model.
+    /// TLS Initial Exec model, or for the TLS Local Exec or Initial Exec models
+    /// on AIX.
----------------
Accidentally repeated the initial comment?
What is the difference between TLS Initial Exec model and Initial Exec models on AIX?

Follow up, I wondering since it is named TPREL to mean Thread pointer relative, which indicates using r13 instead of the get addr call like with dynamic models should we say something like:
```
    /// MO_TPREL_FLAG - If this bit is set the symbol reference is relative to
    /// the thread pointer and is used for TLS Initial Exec , or  TLS Local Exec model.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:816
   auto GetVKForMO = [&](const MachineOperand &MO) {
+    // For LE TLS access on AIX, we have one TOC entry for the symbol (with the
+    // variable offset), which is differentiated by MO_TPREL_FLAG.
----------------
I think we should expand to say local exec explicitly 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:823
+      assert(MO.isGlobal() && "Only expecting a global MachineOperand here!\n");
+      const GlobalValue *GV = MO.getGlobal();
+      TLSModel::Model Model = TM.getTLSModel(GV);
----------------
inline this variable


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:825
+      TLSModel::Model Model = TM.getTLSModel(GV);
+      if (Model == TLSModel::LocalExec)
+        return MCSymbolRefExpr::VariantKind::VK_PPC_AIX_TLSLE;
----------------
Please add an llvm unreachable after this `if`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3349-3351
+  // Aside from the local-exec model, the general-dynamic model is the only
+  // other access model supported for now, so all other GlobalTLSAddress nodes
+  // are lowered with this model.
----------------
I was wanted to suggest a wording that states the key points more explicitly 



================
Comment at: llvm/lib/Target/PowerPC/PPCInstr64Bit.td:1924
+// For the large code model, we match the load (addis+ld) in PPCISelDAGToDAG.
+def : Pat<(PPCaddTls i64:$in, i64:$addr),
+          (ADD8TLS $in, $addr)>;
----------------
Should this have an IsAIX guard?


================
Comment at: llvm/test/CodeGen/PowerPC/aix-tls-le-xcoff-reloc-large.ll:1
+; RUN: llc -verify-machineinstrs -mcpu=pwr4 -mattr=-altivec -mtriple powerpc64-ibm-aix-xcoff \
+; RUN:     -xcoff-traceback-table=false --code-model=large -filetype=obj -o %t.o < %s
----------------
Please change the xcoff test cases to pwr7 for testing consistency


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149722