[PATCH] D149722: [AIX][TLS] Generate 64-bit local-exec access code sequence

[Amy Kwan via Phabricator via llvm-commits]

amyk marked 7 inline comments as done.
amyk added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:155
+      // If the variant kind is VK_PPC_AIX_TLSLE the entry represents the
+      // variable offset for the symbol, we add the relocation specifier @le.
       if (Kind == MCSymbolRefExpr::VariantKind::VK_PPC_AIX_TLSGD ||
----------------
amyk wrote:
> nemanjai wrote:
> > lei wrote:
> > > These comments seem to be something appeneded each time a new condtion was added.  Can we clean up lines 150-155 to be more general and representative? eg
> > > ```
> > > // If the variant kind is a type of VK_PPC_AIX_TLS*, the entry represents the region handle for the symbol and
> > > // we add the corresponding relocation specifier @[m|gd|le].
> > > ```
> > I think it makes sense to unify the `gd/le`, but `m` is the region handle while the other two are variable offsets. Or we can simply state something like:
> > ```
> > // On AIX, we have a region handle (symbol at m) and the variable offset
> > // (symbol@{gd|le}) for TLS variables, depending on the TLS model.
> > ```
> > The details for which is which should be obvious from the code.
> Good suggestion. I'll update the patch to reflect this.
I like this suggestion. I'll update the comment accordingly.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:824
+      TLSModel::Model Model = TM.getTLSModel(MO.getGlobal());
+      if (Model == TLSModel::LocalExec)
+        return MCSymbolRefExpr::VariantKind::VK_PPC_AIX_TLSLE;
----------------
amyk wrote:
> lei wrote:
> > DiggerLin wrote:
> > > ```
> > > TLSModel::Model Model = TM.getTLSModel(MO.getGlobal());
> > >  if (Model == TLSModel::LocalExec)
> > > ```
> > > 
> > > change to 
> > > 
> > > 
> > > ```
> > > if(TM.getTLSModel(MO.getGlobal() == TLSModel::LocalExec))
> > > ```
> > I dont' think we need the tmp variable `Model`, can just update the if stmt thus:
> > ```
> > if (TM.getTLSModel(MO.getGlobal() == TLSModel::LocalExec)
> > ```
> My initial plan was to introduce these variables to be used for later when we implement the other models.
> I'll be more explicit about this next time (especially if I have a future plan on why I added these variables).
Discussed this with Digger.
Digger is OK if we keep this variable, as I have justified above on why I added it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149722