[PATCH] D15300: Fix wrong external references to __emutls_v.*

[Chih-Hung Hsieh via llvm-commits]

chh added inline comments.

================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:394-395
@@ +393,4 @@
+  // __emutls_t.xyz is not in the symbol table.
+  if (TM.Options.EmulatedTLS && GV->getName().startswith("__emutls_v."))
+    return;
+
----------------
rnk wrote:
> Remind me why we bother making an IR-level GlobalVariable if we never want to emit it?
We do emit it, when the controlled variable xyz is visited by this function.
At that time, it is easier to emit both "__emutls_v.xyz" and "__emutls_t.xyz".
Originally, I didn't want to put either of them into symbol table,
but it turned out that it's easier to reuse codegen to access address of "__emutls_v.xyz"
if it is a global variable.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3072
@@ -3071,3 +3071,3 @@
   std::string NameString = ("__emutls_v." + GA->getGlobal()->getName()).str();
   Module *VariableModule = const_cast<Module*>(GA->getGlobal()->getParent());
   StringRef EmuTlsVarName(NameString);
----------------
rnk wrote:
> I guess I didn't pay enough attention on the first review. :(
> 
> We really shouldn't be modifying the IR in TargetLowering. We have some instances of that, but we shouldn't be adding more.
> 
> Long term, if we want to stick with the IR lowering strategy, maybe we should do it as an IR pass. I seem to recall Chandler objected strongly to that, though.
> 
> Anyway, setting the initializer and linkage here for now seems fine.
Agree, I thought about a special pass for TLS too, until I found the target dependent LowerGlobalTLSAddres.
So, here LowerToTLSEmulateMode is only a target independent special case of LowerGlobalTLSAddres.


================
Comment at: lib/CodeGen/SelectionDAG/TargetLowering.cpp:3088
@@ -3078,2 +3087,3 @@
+  }
   Entry.Node = DAG.getGlobalAddress(EmuTlsVar, dl, PtrVT);
   Entry.Ty = VoidPtrType;
----------------
rnk wrote:
> If we only need the global variable to make this GlobalAddress node, there are other ways to do that, like DAG.getExternalSymbol and DAG.getMCSymbol.
I tried to do that, but maybe I did not make it all correct for an external symbol.
What I found out after my initial implementation is that get address of an external symbol would fail to link if the symbol is defined with internal linkage, for some linkers. That's what this patch to fix. So I would now make the symbol as real as necessary here to generate code to get the address.



http://reviews.llvm.org/D15300