[PATCH] [PowerPC] Replace foul hackery with real calls to __tls_get_addr

[Bill Schmidt]

> I don't follow.  VK_PLT is a decorator for @PLT, and I already have tests for ensuring @PLT becomes R_PPC_PLTREL24 (ppc-reloc.s).  Isn't a sufficient test for this simply to ensure that @PLT is generated when that code path is followed?  Do you instead expect that the inline assembler will recognize a call to __tls_get_addr() and itself add VK_PLT?  The other relocations for PLT are identical to those of powerpc64 in this case.  Okay, maybe a test to generate R_PPC_GOT_TLSLD/R_PPC_GOT_TLSGD instead of R_PPC_GOT_TLSLD_LO/R_PPC_GOT_TLSGD_LO might be prudent, and I can readily add, but that's not what you requested.

Normally, @PLT is how you get R_PPC_PLTREL24, but that is apparently not the case here.  You added some special-case code in PPCAsmPrinter.cpp that specifically places the VK_PLT on the __tls_get_addr symbol ONLY, which is never emitted with @plt.  (It's in the deleted chunks in this patch.)  This appears to be a way of forcing the relocation that you want, and this path is not tested in your test cases.

>From what I have been able to piece together, I believe that placing MO_PLT_OR_STUB on the TGA operand early will eventually cause VK_PLT to be used, and hence generate the correct relocation.  I can't tell whether this regresses your code or not, though, because there wasn't a test.

The way I know there isn't a test is that I only discovered your code when I was inspecting the patch.  I deleted your VK_PLT workaround with impunity and none of the tests failed.  When I spotted that I added the MO_PLT_OR_STUB code.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:782
@@ -781,3 +781,3 @@
   case PPCISD::CALL:            return "PPCISD::CALL";
   case PPCISD::CALL_NOP:        return "PPCISD::CALL_NOP";
   case PPCISD::MTCTR:           return "PPCISD::MTCTR";
----------------
hfinkel wrote:
> Please add CALL_TLS, CALL_NOP_TLS here.
Good catch, thanks!

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:1767
@@ +1766,3 @@
+
+    std::pair<SDValue, SDValue> CallResult = LowerCallTo(CLI);
+    SDValue TLSAddr = CallResult.first;
----------------
hfinkel wrote:
> This block of code seems the same as the one added above. Can you please refactor this?
Sure, good idea.

================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:3741
@@ +3740,3 @@
+        assert(!needIndirectCall && "Indirect call to __tls_get_addr???");
+        SDNode *AddI = Chain.getNode()->getOperand(2).getNode();
+        SDValue TGTAddr = AddI->getOperand(1);
----------------
rafael wrote:
> This patch is nice, but this part looks a bit too clever. Could you pass TGTAddr down the callchain maybe?
I couldn't think of a way to pass it down directly -- I'd certainly welcome a suggestion, as I agree with your assessment.  Hacking it onto the Args list seemed like a worse idea.  I can't really do more with the chain argument because it's used by the call-lowering code.  Thoughts?

At least the present solution is guaranteed to work because we're just looking at the sequence generated by the call-lowering, which is still in progress.  I.e., no optimizations or anything can get in the way.

http://reviews.llvm.org/D6209