[PATCH] D152669: [AIX][TLS] Generate 32-bit local-exec access code sequence

[Amy Kwan via Phabricator via llvm-commits]

amyk marked 5 inline comments as done.
amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:616-626
+  if (MIOpc == PPC::GETtlsTpointer32AIX)
+    return Ctx
+        .getXCOFFSection(".__get_tpointer", SectionKind::getText(),
+                         XCOFF::CsectProperties(XCOFF::XMC_PR, XCOFF::XTY_ER))
+        ->getQualNameSymbol();
+  else
+    return Ctx
----------------
nemanjai wrote:
> Can this not be something like:
> ```
> StringRef SymName = MIOpc == PPC::GETtlsTpointer32AIX ?
>   ".__get_tpointer" : ".__tls_get_addr";
> 
> return Ctx.getXCOFFSection(SymName, SectionKind::getText(),
>           XCOFF::CsectProperties(XCOFF::XMC_PR, XCOFF::XTY_ER))
>   ->getQualNameSymbol();
> ```
Thanks for the suggestion.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:628
+
+void PPCAsmPrinter::EmitAIXTlsCallHelper(const MachineInstr *MI) {
+  assert(Subtarget->isAIXABI() &&
----------------
nemanjai wrote:
> Can this not be a `static` file-scope function?
I like this suggestion and I looked into this. I believe it cannot as I need to use `EmitToStreamer()`, however I could be wrong/missing something.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:1742
+  case PPCISD::GET_TPOINTER:
+                                return "PPCISD::GET_TPOINTER";
   case PPCISD::ADDI_TLSGD_L_ADDR: return "PPCISD::ADDI_TLSGD_L_ADDR";
----------------
nemanjai wrote:
> Not sure if it was `clang-format` that suggested this, but please put these on the same line so that it matches the surrounding formatting.
Yeah, this was the result of `clang-format`. I'll move this line back.


================
Comment at: llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp:59
         IsPCREL = isPCREL(MI);
+        bool IsTLSTPRelMI = MI.getOpcode() == PPC::GETtlsTpointer32AIX;
 
----------------
nemanjai wrote:
> Please add a comment since I don't think the name of the variable can adequately represent what it is for. Something like:
> ```
> // There are a number of slight differences in code generation
> // when we call .__get_tpointer (32-bit AIX TLS).
> ```
Comment added.


================
Comment at: llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp:158
+            // which will return the thread pointer into r3.
+            BuildMI(MBB, I, DL, TII->get(Opc2), GPR3);
+          }
----------------
nemanjai wrote:
> Nit: please refrain from putting single statements in braces.
Good catch. I did this also in PPCISelLowering.cpp, so I'll remove those accordingly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152669