[PATCH] D150367: [AIX][TLS] Generate optimized local-exec access code sequence using X-Form loads/stores

[Amy Kwan via Phabricator via llvm-commits]

amyk added a comment.

Group code review comments.



================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:732
+  // address comes from the computation feeding the load.
   if (!Offset.isUndef())
     return false;
----------------
These two checks for the offset can be combined.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:738
 
+  // This optimization can also be performed on AIX if the thread pointer is
+  // being added to the variable offset (loaded from the TOC).
----------------
Update this comment for `GET_TPOINTER`, and just mention it's produced for AIX on 32-bit mode.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:749
+  // memory operations into their X-Form counterparts.
+  const PPCSubtarget &Subtarget =
+      CurDAG->getMachineFunction().getSubtarget<PPCSubtarget>();
----------------
- Implement a static helper function, to check if the node of a thread pointer acquisition node (if one of the operands is the thread pointer - either `R13`, or `__get_tpointer`).
- If conditions are satisfied, return `true`.
- Otherwise, return `false` at the end of the static helper function.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:829
+      if (isSExt)
+        Opcode = (RegVT == MVT::i32) ? PPC::LHAXTLS_32 : PPC::LHAXTLS;
+      else
----------------
A ternary condition for `isSExt` for here and below might be better.


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.h:279
 
+  MCRegister getThreadPointerRegister() const {
+    assert((is64BitELFABI() || isAIXABI()) &&
----------------
amyk wrote:
> Is it appropriate to add this in this patch? Or should this be separate?
Discussed this with the team. It is appropriate to add this here.


================
Comment at: llvm/test/CodeGen/PowerPC/aix-tls-le-ldst-double.ll:14
 ; RUN:      | FileCheck %s --check-prefix=LARGE32
+; RUN: llc  -O0 -verify-machineinstrs -mcpu=pwr7 -ppc-asm-full-reg-names \
+; RUN:      -mtriple powerpc64-ibm-aix-xcoff < %s | FileCheck %s \
----------------
Instead of adding `-O0` lines to every test, we can add one file to show the `-O0` codegen.
That way, we can remove the `-O0` run lines from these existing test cases.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150367