[PATCH] D144941: [m68k] Add TLS Support

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1399
+                             unsigned TargetFlags) {
+  // In general dynamic and local dynamic access models, to access a variable in
+  // Thread Local Storage (TLS), we typically use the function __tls_get_addr.
----------------
None of this is m68k-specific (with the exception of using unsigned instead of unsigned long), these long comments are just a repeat of how ELF TLS works in general. I don't think they belong here.


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1420
+  // 3. Just call `__tls_get_addr`
+  return DAG.getNode(M68kISD::TLSGetAddr, SDLoc(GA), GA->getValueType(0), Arg);
+}
----------------
Skip the custom node / pseudo and use LowerCallTo?


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1467
+  SDValue GOT = DAG.getGLOBAL_OFFSET_TABLE(MVT::i32);
+  SDValue Tp = DAG.getNode(M68kISD::TLSReadTp, SDLoc(GA), MVT::i32);
+  SDValue TGA =
----------------
Skip the custom node / pseudo and use LowerCallTo?


================
Comment at: llvm/lib/Target/M68k/M68kISelLowering.cpp:1484
+  // to allocate a slot in GOT.
+  SDValue Tp = DAG.getNode(M68kISD::TLSReadTp, SDLoc(GA), MVT::i32);
+  SDValue TGA =
----------------
Ditto


================
Comment at: llvm/lib/Target/M68k/M68kInstrArithmetic.td:317
+// for global values that are allocated in thread-local storage, i.e.:
+//   t8: i32 = M68kISD::ADD GLOBAL_OFFSET_TABLE, TargetGlobalTLSAddress:i32<ptr @myvar>
+//     ====>
----------------
Presumably because you're abusing M68kISD::ADD, which has type MxSDT_BiArithCCROut:
```
// RES, CCR <- op LHS, RHS
def MxSDT_BiArithCCROut : SDTypeProfile<2, 2, [
  /* RES */ SDTCisInt<0>,
  /* CCR */ SDTCisVT<1, i8>,
  /* LHS */ SDTCisSameAs<0, 2>,
  /* RHS */ SDTCisSameAs<0, 3>
]>;
```

You're using it as if it only had one i32 result, but it's `iN,i8 = M68kISD::ADD iN, iN`. Since you don't care about CCR here presumably you can just use a normal ISD::ADD instead, but if you do then use the right types.


================
Comment at: llvm/lib/Target/M68k/M68kInstrCompiler.td:133
+let Defs = [A0], isCall = 1 in {
+
+def Pseudo_TLSGetAddr: MxPseudo<(outs), (ins XR32:$src)>;
----------------
Why the blank lines?


================
Comment at: llvm/lib/Target/M68k/M68kInstrCompiler.td:134
+
+def Pseudo_TLSGetAddr: MxPseudo<(outs), (ins XR32:$src)>;
+def Pseudo_TLSReadTp : MxPseudo<(outs), (ins)>;
----------------
: needs a space


================
Comment at: llvm/lib/Target/M68k/M68kInstrCompiler.td:134
+
+def Pseudo_TLSGetAddr: MxPseudo<(outs), (ins XR32:$src)>;
+def Pseudo_TLSReadTp : MxPseudo<(outs), (ins)>;
----------------
jrtc27 wrote:
> : needs a space
Surely this function call clobbers all call-clobbered registers like any other architecture's __tls_get_addr? Glancing at glibc, the version used for statically-linked binaries is implemented in C so won't be doing anything special to preserve registers.


================
Comment at: llvm/lib/Target/M68k/M68kInstrCompiler.td:135
+def Pseudo_TLSGetAddr: MxPseudo<(outs), (ins XR32:$src)>;
+def Pseudo_TLSReadTp : MxPseudo<(outs), (ins)>;
+
----------------
AFAICT this is also implemented in C and thus clobbers the usual call-clobbered registers.


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kBaseInfo.h:135
+
+  MO_TLSGD,
+  MO_TLSLD,
----------------
Comment


================
Comment at: llvm/lib/Target/M68k/MCTargetDesc/M68kELFObjectWriter.cpp:87
+
+    MAP_RELOC_TYPE(VK_TLSGD, R_68K_TLS_GD)
+    MAP_RELOC_TYPE(VK_TLSLDM, R_68K_TLS_LDM)
----------------
These should be deindented, but the other variants don't do this so now it's all an inconsistent mess.


================
Comment at: llvm/test/CodeGen/M68k/TLS/tlsie.ll:19
+
+; RELOC-LABEL: Section (3) .rela.text
+; RELOC-DAG: 0xC R_68K_32 __m68k_read_tp 0x0
----------------
These shouldn't be in UTC tests. Besides, it's implied by the assembly, and if you want to test the relocation emission then write MC tests for that part.


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

https://reviews.llvm.org/D144941