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

[Victor Huang via Phabricator via llvm-commits]

NeHuang added a comment.

Overall looks fine to me, but of course please wait to hear from Stefan and Nemanja.
Some nits added.



================
Comment at: llvm/lib/Target/PowerPC/PPC.h:126
+    /// Fix up is VK_PPC_GOT_TPREL_PCREL
+    MO_GOT_TPREL_PCREL_FLAG = MO_PCREL_FLAG | MO_GOT_FLAG | MO_TPREL_FLAG,
+
----------------
nit: use `MO_GOT_FLAG | MO_TPREL_FLAG | MO_PCREL_FLAG` to be consistent with order in relocation name and the flag name. You can also update it for `MO_GOT_TLSGD_PCREL_FLAG` 


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll:13
+
+define i32* @InitialExec() {
+; CHECK-S-LABEL: InitialExec:
----------------
nit: change to `InitialExecAddressLoad` to better describe the case 


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll:23
+; CHECK-O-NEXT:    14 6a 63 7c                  	add 3, 3, 13
+; CHECK-O-NEXT:    0000000000000009:  R_PPC64_TLS	x
+; CHECK-O-NEXT:    20 00 80 4e                  	blr
----------------
Please add a comment here for the one-byte displacement.  Same for line 39.


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tls-initial-exec.ll:29
+
+define i32 @InitialExecLoad() {
+; CHECK-S-LABEL: InitialExecLoad:
----------------
nit: change to `InitialExecValueLoad` 


================
Comment at: llvm/test/MC/PowerPC/pcrel-tls-initial-exec-address-load-reloc.s:7
+# This test checks that on Power PC we can correctly convert x at got@tprel at pcrel
+# and x at tls@pcrel into R_PPC64_GOT_TPREL_PCREL34, and R_PPC64_TLS for initial
+# exec relocations with address loaded.
----------------
R_PPC64_TLS (with one-byte displacement)


================
Comment at: llvm/test/MC/PowerPC/pcrel-tls-initial-exec-address-load-reloc.s:15
+
+  .text
+	.abiversion 2
----------------
- You can only keep necessary assembly in the test and remove the comments. 
- Rename the function.
```
InitialExecAddressLoad:
  pld 3, x at got@tprel at pcrel(0), 1
  add 3, 3, x at tls@pcrel
  blr
```



================
Comment at: llvm/test/MC/PowerPC/pcrel-tls-initial-exec-value-load-reloc.s:7
+# This test checks that on Power PC we can correctly convert x at got@tprel at pcrel
+# and x at tls@pcrel into R_PPC64_GOT_TPREL_PCREL34, and R_PPC64_TLS for initial
+# exec relocations with the value loaded.
----------------
ditto


================
Comment at: llvm/test/MC/PowerPC/pcrel-tls-initial-exec-value-load-reloc.s:21
+	.type	InitialExecLoad, at function
+InitialExecLoad:                        # @InitialExecLoad
+.Lfunc_begin0:
----------------
ditto


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81947