[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 inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3360
+        if (GVType->isSized() && !GVType->isEmptyTy() &&
+            GV->getParent()->getDataLayout().getTypeAllocSize(GVType) <
+                AIXSmallTlsPolicySizeLimit)
----------------
amyk wrote:
> 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.
No problem; thanks for addressing.


================
Comment at: llvm/lib/Target/PowerPC/PPCMCInstLower.cpp:100
+    assert(Model == TLSModel::LocalExec &&
+           "Only expecting local-exec accesses!");
+    RefKind = MCSymbolRefExpr::VK_PPC_AIX_TLSLE;
----------------
amyk wrote:
> 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.
Considering that this change to set the `RefKind` was not needed for the base local-exec code model enablement, there is no reason to expect that initial-exec code actually gets here (and if it does, we should want to inspect the result of what this function does).

In other words, this code only became necessary for local-exec when we started applying relocations on the instruction operand, and since initial-exec TLS relocations on such operands are not allowed, the assert should stay. Actually, the old wording is correct with this rationale (so let's go with the old wording).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600