[PATCH] D81947: [PowerPC][PCRelative] Thread Local Storage Support for Initial Exec

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Overall this seems fine. I have a number of comments but they are mostly minor.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp:333
+  const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(Expr);
+  bool isPCRel = SRE->getKind() == MCSymbolRefExpr::VK_PPC_TLS_PCREL;
+  Fixups.push_back(MCFixup::create(isPCRel ? 1 : 0, Expr,
----------------
nit:
`isPCRel` -> `IsPCRel`


================
Comment at: llvm/lib/Target/PowerPC/PPC.h:114
+    /// Fix up is VK_PPC_GOT_TPREL_PCREL
+    MO_TPREL_FLAG = 32,
+
----------------
Why 32 and not 16?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3037
   if (Model == TLSModel::InitialExec) {
-    SDValue TGA = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0, 0);
-    SDValue TGATLS = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
-                                                PPCII::MO_TLS);
-    SDValue GOTPtr;
-    if (is64bit) {
-      setUsesTOCBasePtr(DAG);
-      SDValue GOTReg = DAG.getRegister(PPC::X2, MVT::i64);
-      GOTPtr = DAG.getNode(PPCISD::ADDIS_GOT_TPREL_HA, dl,
-                           PtrVT, GOTReg, TGA);
+    bool isPCRel = Subtarget.isUsingPCRelativeCalls();
+    SDValue TGA = isPCRel ? DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
----------------
nit:
`isPCRel` -> `IsPCRel`


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3041
+                                                           PPCII::MO_TPREL_FLAG)
+                          : DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0, 0);
+    SDValue TGATLS =
----------------
The only difference between the two global addresses is the flags that are added in the PCRel case.
It may be clearer to write it as: (ignore my spacing)
```
    SDValue TGA = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
	​                                     isPCRel ? (PPCII::MO_PCREL_FLAG | PPCII::MO_TPREL_FLAG)  : 0)
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:3045
+                      GV, dl, PtrVT, 0, PPCII::MO_TLS | PPCII::MO_PCREL_FLAG)
+                : DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0, PPCII::MO_TLS);
+    SDValue TPOffset;
----------------
Same thing here:
```
SDValue TGATLS = DAG.getTargetGlobalAddress(GV, dl, PtrVT, 0,
    isPCRel ? PPCII::MO_TLS | PPCII::MO_PCREL_FLAG : PPCII::MO_TLS)
```


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tls-inital-exec.ll:6
+; RUN:   -mcpu=future -ppc-asm-full-reg-names --filetype=obj < %s | \
+; RUN:   llvm-objdump --mcpu=future -dr - | FileCheck %s --check-prefix=CHECK-O
+
----------------
nit:
Should probably use `pwr10` instead of `future` in both places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81947