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

[Amy Kwan via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/MC/MCExpr.h:302
     VK_PPC_AIX_TLSGDM,      // symbol at m
+    VK_PPC_AIX_TLSLE,       // symbol at le
     VK_PPC_GOT_TLSLD,       // symbol at got@tlsld
----------------
lei wrote:
> nemanjai wrote:
> > lei wrote:
> > > I realize this was done similar to `VK_PPC_AIX_TLS*`. but the symbol outputed seems to be too general and feels like we should fix this.  Shouldn't this be something more along the lines of `symbol at aix@tlsle`?
> > These are actual strings printed in ASM (i.e. the textual representation of the relocation) so we can't freely add `aix` in there since the assembler would not know what to do with that.
> I see.  Thank-you.
That's correct. Thanks for answering this question, Nemanja.


================
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 ||
----------------
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.


================
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;
----------------
lei wrote:
> 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).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3332
 
-  // The general-dynamic model is the only access model supported for now, so
-  // all the GlobalTLSAddress nodes are lowered with this model.
+  if (Model == TLSModel::LocalExec) {
+    if (Is64Bit) {
----------------
lei wrote:
> I don't think `Model` is used anywhere else... can probably inline that as well.
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).


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3333
+  if (Model == TLSModel::LocalExec) {
+    if (Is64Bit) {
+      // For local-exec on AIX (64-bit), the sequence that is generated involves
----------------
lei wrote:
> `Is64Bit` is only used once.  Please inline and remove this variable def.
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149722