[PATCH] D45520: [PowerPC] add secure plt support for TLS symbols

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I think overall, it would be good to explain the semantics of this patch. Maybe even provide a reference to a document that explains it if it isn't something that can be summarized into a comment in the review (or a comment in the code).

Also, I imagine you'll want to add some code to `getGlobalBaseReg()` to disable shrink-wrapping since you're adding a clobber of the LR without introducing any frame references, so shrink-wrapping may move the prologue out of the entry.



================
Comment at: lib/Target/PowerPC/InstPrinter/PPCInstPrinter.cpp:445
   const MCOperand &Op = MI->getOperand(OpNo);
-  const MCSymbolRefExpr &refExp = cast<MCSymbolRefExpr>(*Op.getExpr());
-  O << refExp.getSymbol().getName();
+  const MCSymbolRefExpr *refExp = nullptr;
+  const MCConstantExpr *constExp = nullptr;
----------------
Nit: naming convention - variables start with an upper case letter. I realize it was that way, but since you're changing its type, might as well change its name.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4011
     break;
+  case PPCISD::ADDI_TLSLD_L_ADDR:
+  case PPCISD::ADDI_TLSGD_L_ADDR: {
----------------
A comment explaining why this is needed would be great.


================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:4019
 
+      getGlobalBaseReg();
+  }
----------------
I think it's pertinent to add a comment to calls like this one outlining why the virtual SDNode returned by this function does not need to have a use and it still won't end up being DCE'd.


================
Comment at: test/CodeGen/PowerPC/ppc32-secure-plt-tls.ll:13
+!0 = !{i32 7, !"PIC Level", i32 2}
+
+; SECURE-PLT-TLS:       mflr 30
----------------
I imagine there's supposed to be code up here that sets up the base pointer. Do we not want to check for that code?


Repository:
  rL LLVM

https://reviews.llvm.org/D45520