[PATCH] D82315: [PowerPC][PCRelative] Thread Local Storage Support for General Dynamic

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

Bunch of nits but overall the idea is fine. I only had one real concern with respect to the setup of `InReg` and you can find that in the comments.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp:517
+  // When using PCRelative addressing for TLS dynamic models, adjust the call
+  // assembly print to be "__tls_get_addr at notoc" according to the ABI.
+  if (RefExp->getKind() == MCSymbolRefExpr::VK_PPC_NOTOC)
----------------
nit:
That comment is confusing a couple of things.
The reason we have to do all of this special handling is because we don't want:
```
__tls_get_addr(x at tlsgd)@notoc
```
We want:
```
__tls_get_addr at notoc(x at tlsgd)
```
Maybe:
```
The variant kind VK_PPC_NOTOC needs to be handled as a special case because we do not want the assembly to print out the @notoc at the end like __tls_get_addr(x at tlsgd)@notoc. Instead we want it to look like __tls_get_addr at notoc(x at tlsgd).
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:468
+
+  if (MI->getOperand(2).getTargetFlags() == PPCII::MO_GOT_TLSGD_PCREL_FLAG) {
+    Kind = MCSymbolRefExpr::VK_PPC_NOTOC;
----------------
nit:
Can you add an assert above to make sure that this MI needs to have at least 3 operands?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:474
+    Opcode = PPC::BL8_NOP_TLS;
+  }
   const Module *M = MF->getFunction().getParent();
----------------
Can we just set whatever is in the `else` condition as the default for `Kind` and `Opcode`?
I think this may be more of a style thing but I like having defaults for variables since that way it is harder for future code to accidentally end up with a situation where someone adds an `else if` and forgets to define one of them.


================
Comment at: llvm/lib/Target/PowerPC/PPCInstrInfo.td:324
 def PPCmatpcreladdr : SDNode<"PPCISD::MAT_PCREL_ADDR", SDTIntUnaryOp, []>;
+def PPCtlsdynamatpcreladdr : SDNode<"PPCISD::TLS_DYNAMIC_MAT_PCREL_ADDR", SDTIntUnaryOp, []>;
 
----------------
nit:
Line too long?


================
Comment at: llvm/lib/Target/PowerPC/PPCTLSDynamicCall.cpp:83
+          InReg = MI.getOperand(1).getReg();
+
         DebugLoc DL = MI.getDebugLoc();
----------------
I'm a little worried about this setup.
In the case where `IsTLSGDPCREL == true` we end up with `InReg` not being assigned a register.
However, in that case we still do this:
```
const Register OrigRegs[] = {OutReg, InReg, GPR3};
// Some more code here...
LIS->repairIntervalsInRange(&MBB, First, Last, OrigRegs);
```
Is this a safe thing to do?
Perhaps in that scenario it would be better to only have 
```
const Register OrigRegs[] = {OutReg, GPR3};
```



================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tls-general-dynamic.ll:7
+; RUN:   --filetype=obj < %s | \
+; RUN:   llvm-objdump --mcpu=future -dr - | FileCheck %s --check-prefix=CHECK-O
+
----------------
nit:
Start using `mcpu=pwr10` instead of future.


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-tls-general-dynamic.ll:9
+
+ at x = external thread_local global i32, align 4
+
----------------
nit:
Add a little description of the test here.


================
Comment at: llvm/test/MC/PowerPC/pcrel-tls-general-dynamic-address-load-reloc.s:22
+	.type	_Z8tls_testv, at function
+_Z8tls_testv:                           # @_Z8tls_testv
+.Lfunc_begin0:
----------------
nit:
Try to avoid using C++ mangled names. They are harder to read.
I would say rename this function and the function in the test below to describe what the function does.
eg. `GDAddrLoad` or `GDGetAddr`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82315