[PATCH] D143708: [RISCV] Support emulated TLS

[Vitaly Cheptsov via Phabricator via llvm-commits]

vit9696 added a comment.

Sorry for the delay, got a bit sick here. I mostly acked the requested changes and left comments on the ones I believe should not be applied. Will try to prepare the V2 by the end of the week.



================
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
----------------
jrtc27 wrote:
> Why do we need optimisation tests?
To make sure there are no oddities with the optimisation passes against emutls.


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:20
+
+; NoEMU-NOT: __emutls
+
----------------
jrtc27 wrote:
> 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?
It is not implied. Negative checking is mandatory for any good test, in my opinion.


================
Comment at: llvm/test/CodeGen/RISCV/emutls_generic.ll:22
+
+; Make sure that TLS symbols are emitted in expected order.
+
----------------
jrtc27 wrote:
> Who cares about order?
The original test does, but I kind of expect this too.


================
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:
----------------
jrtc27 wrote:
> What on earth is .rdata?
.rdata is used in PE/COFF instead of .rodata. As long as Microsoft adds RISC-V support this is how this code may be generated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143708