[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 accepted this revision.
hubert.reinterpretcast added a comment.

LGTM with request to add a comment.



================
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:
> 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).
It seems we run this code for TOC reference cases before we replace with a reference to the TOC entry. That may not be ideal, but it works.

We can leave the code as-is (with the `if` check). A comment would help:
For the local-exec TLS model, we may generate the offset from the TLS base as an immediate operand (instead of using a TOC entry); set the relocation type in case the result is used for purposes other than a TOC reference. In TOC reference cases, this result is discarded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600