[PATCH] D91432: Move GlobalTLSAddress handling to WebAssemblyISelLowering. NFC

[Thomas Lively via Phabricator via llvm-commits]

tlively added a comment.

Yes, this looks better than what we had before. I think it would be generally better to use TableGen patterns rather than generating the MachineNode directly in C++.



================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelDAGToDAG.cpp:126
 
-  case ISD::GlobalTLSAddress: {
-    const auto *GA = cast<GlobalAddressSDNode>(Node);
-
-    if (!MF.getSubtarget<WebAssemblySubtarget>().hasBulkMemory())
-      report_fatal_error("cannot use thread-local storage without bulk memory",
-                         false);
-
-    // Currently Emscripten does not support dynamic linking with threads.
-    // Therefore, if we have thread-local storage, only the local-exec model
-    // is possible.
-    // TODO: remove this and implement proper TLS models once Emscripten
-    // supports dynamic linking with threads.
-    if (GA->getGlobal()->getThreadLocalMode() !=
-            GlobalValue::LocalExecTLSModel &&
-        !Subtarget->getTargetTriple().isOSEmscripten()) {
-      report_fatal_error("only -ftls-model=local-exec is supported for now on "
-                         "non-Emscripten OSes: variable " +
-                             GA->getGlobal()->getName(),
-                         false);
-    }
-
-    SDValue TLSBaseSym = CurDAG->getTargetExternalSymbol("__tls_base", PtrVT);
-    SDValue TLSOffsetSym = CurDAG->getTargetGlobalAddress(
-        GA->getGlobal(), DL, PtrVT, GA->getOffset(),
-        WebAssemblyII::MO_TLS_BASE_REL);
-
-    MachineSDNode *TLSBase =
-        CurDAG->getMachineNode(GlobalGetIns, DL, PtrVT, TLSBaseSym);
-    MachineSDNode *TLSOffset =
-        CurDAG->getMachineNode(ConstIns, DL, PtrVT, TLSOffsetSym);
-    MachineSDNode *TLSAddress = CurDAG->getMachineNode(
-        AddIns, DL, PtrVT, SDValue(TLSBase, 0), SDValue(TLSOffset, 0));
-    ReplaceNode(Node, TLSAddress);
-    return;
-  }
-
   case ISD::INTRINSIC_WO_CHAIN: {
     unsigned IntNo = cast<ConstantSDNode>(Node->getOperand(0))->getZExtValue();
----------------
I think this has a lowering hook in `WebAssemblyISelLowering.cpp` as well and could also be moved, although that could be a follow-up patch.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1315-1318
+  SDValue BaseAddr(
+      DAG.getMachineNode(GlobalGet, DL, PtrVT,
+                         DAG.getTargetExternalSymbol(BaseName, PtrVT)),
+      0);
----------------
I think the pattern used down in the next method is more readable.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrInfo.td:355-359
+def : Pat<(i32 (WebAssemblywrapper tglobaltlsaddr:$addr)),
+          (CONST_I32 tglobaltlsaddr:$addr)>, Requires<[HasAddr32]>;
+def : Pat<(i64 (WebAssemblywrapper tglobaltlsaddr:$addr)),
+          (CONST_I64 tglobaltlsaddr:$addr)>, Requires<[HasAddr64]>;
+
----------------
IIUC, these aren't currently used because the lowering generates a MachineNode directly, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91432