[PATCH] D143708: [RISCV] Add emulated TLS supported

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4572
                                                    SelectionDAG &DAG) const {
+  GlobalAddressSDNode *GA = cast<GlobalAddressSDNode>(Op);
+  if (DAG.getTarget().useEmulatedTLS())
----------------
Hoist N instead


================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:4574
+  if (DAG.getTarget().useEmulatedTLS())
+    return LowerToTLSEmulatedModel(GA, DAG);
   GlobalAddressSDNode *N = cast<GlobalAddressSDNode>(Op);
----------------
Newline after


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:2
+; RUN: llc < %s -emulated-tls -mtriple=riscv64-gnu-linux -relocation-model=pic \
+; RUN:     | FileCheck -check-prefix=RISCV_64 %s
+; RUN: llc < %s -emulated-tls -mtriple=riscv64-gnu-linux -relocation-model=pic -O3 \
----------------
_ in CHECK prefixes is unusual, but normally we do RV32/64


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:3
+; RUN:     | FileCheck -check-prefix=RISCV_64 %s
+; RUN: llc < %s -emulated-tls -mtriple=riscv64-gnu-linux -relocation-model=pic -O3 \
+; RUN:     | FileCheck -check-prefix=RISCV_64 %s
----------------
Why do we need optimisation tests?


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:8
+
+; RUN: llc < %s -emulated-tls -mtriple=riscv32-gnu-linux -relocation-model=pic \
+; RUN:     | FileCheck -check-prefix=RISCV_32 %s
----------------
32 before 64


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:20
+
+; NoEMU-NOT: __emutls
+
----------------
Why's that in an emutls test? And isn't that implied by our TLS tests not having anything to do with emutls in the generated code?


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:22
+
+; Make sure that TLS symbols are emitted in expected order.
+
----------------
Who cares about order?


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:28
+
+define ptr @get_external_x() {
+entry:
----------------
These need codegen check lines, for which you should be using update_llc_test_checks.py


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:43
+
+; RISCV_64-LABEL:  get_external_x:
+; RISCV_64:      __emutls_v.external_x
----------------
This isn't readable, at least indent things properly, though I forget if update_llc_test_checks.py can do globals in assembly... I think not?


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:44
+; RISCV_64-LABEL:  get_external_x:
+; RISCV_64:      __emutls_v.external_x
+; RISCV_64:      __emutls_get_address
----------------
Distinct lack of -NEXT everywhere


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:54
+; RISCV_64-NOT:   __emutls_v.external_x:
+; RISCV_64:        .data{{$}}
+; RISCV_64:        .globl __emutls_v.external_y
----------------
Why does the lack of a suffix matter? Seems a bit odd to pick this specifically.


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:63
+; RISCV_64-NOT:    __emutls_v.external_x:
+; RISCV_64:        .section .r{{o?}}data,
+; RISCV_64-LABEL:  __emutls_t.external_y:
----------------
What on earth is .rdata?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143708