[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/PPCMCInstLower.cpp:100
+    assert(Model == TLSModel::LocalExec &&
+           "Only expecting local-exec accesses!");
+    RefKind = MCSymbolRefExpr::VK_PPC_AIX_TLSLE;
----------------
hubert.reinterpretcast wrote:
> 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.
Thanks for discussing with me offline regarding this. I've added the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155600