[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)

[Amy Kwan via Phabricator via llvm-commits]

amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3360
+        if (GVType->isSized() && !GVType->isEmptyTy() &&
+            GV->getParent()->getDataLayout().getTypeAllocSize(GVType) <
+                AIXSmallTlsPolicySizeLimit)
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > Sorry for the extra churn: with the new name, we should use `<=` here (and adjust the value to `32751`).
> This comment has not been addressed.
Ah, I am very sorry. I thought I had addressed it but it turns out that I did not.
I have addressed it now.


================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:100
+    assert(Model == TLSModel::LocalExec &&
+           "Only expecting local-exec accesses!");
+    RefKind = MCSymbolRefExpr::VK_PPC_AIX_TLSLE;
----------------
hubert.reinterpretcast wrote:
> Suggested wording change.
@hubert.reinterpretcast After rebasing the patch, initial-exec also uses the same flag and code path. Thus, I don't think the `assert()` is appropriate here anymore.

I've updated this to just a check for local-exec and just setting the `RefKind` in this condition, which I think should be OK since it won't get set in the initial-exec case.
```
diff --git a/llvm/lib/Target/PowerPC/PPCMCInstLower.cpp b/llvm/lib/Target/PowerPC/PPCMCInstLower.cpp
index 92a1311e57a8..e2bb28b397c0 100644
--- a/llvm/lib/Target/PowerPC/PPCMCInstLower.cpp
+++ b/llvm/lib/Target/PowerPC/PPCMCInstLower.cpp
@@ -96,9 +96,8 @@ static MCOperand GetSymbolRef(const MachineOperand &MO, const MCSymbol *Symbol,
   else if (MO.getTargetFlags() == PPCII::MO_TPREL_FLAG) {
     assert(MO.isGlobal() && "Only expecting a global MachineOperand here!");
     TLSModel::Model Model = TM.getTLSModel(MO.getGlobal());
-    assert(Model == TLSModel::LocalExec &&
-           "Only local-exec accesses are handled!");
-    RefKind = MCSymbolRefExpr::VK_PPC_AIX_TLSLE;
+    if (Model == TLSModel::LocalExec)
+      RefKind = MCSymbolRefExpr::VK_PPC_AIX_TLSLE;
   }

   const MachineInstr *MI = MO.getParent();
```
I was wondering if you had any thoughts or objections to this.


================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:97
+  else if (MO.getTargetFlags() == PPCII::MO_TPREL_FLAG) {
+    assert(MO.isGlobal() && "Only expecting a global MachineOperand here!\n");
+    TLSModel::Model Model = TM.getTLSModel(MO.getGlobal());
----------------
hubert.reinterpretcast wrote:
> hubert.reinterpretcast wrote:
> > `assert`s print the spelling of the expression, so the `\n` is not needed.
> This comment has not been addressed.
Sorry, addressed now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600