[PATCH] D11103: Remove access to the DataLayout in the TargetMachine

[Mehdi AMINI]

joker.eph added a comment.

Thanks for the review Raphael.


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinter.cpp:143
@@ +142,3 @@
+// Do not use the cached DataLayout because some client use it without a Module
+// (llmv-dsymutil, llvm-dwarfdump).
+unsigned AsmPrinter::getPointerSize() const { return TM.getPointerSize(); }
----------------
rafael wrote:
> For another patch, but could the AsmPrinter constructor take the pointer size?
> 
Yes it is an option, I'll look into it.

================
Comment at: lib/ExecutionEngine/MCJIT/MCJIT.cpp:71
@@ -70,3 +70,3 @@
              std::shared_ptr<RuntimeDyld::SymbolResolver> Resolver)
-    : ExecutionEngine(*TM->getDataLayout(), std::move(M)), TM(std::move(TM)),
+    : ExecutionEngine(TM->createDataLayout(), std::move(M)), TM(std::move(TM)),
       Ctx(nullptr), MemMgr(std::move(MemMgr)),
----------------
rafael wrote:
> Can't it use the DL from the module?
The problem is that the JIT supports adding a module but also removing a module from its list. Keeping a reference to the DL of the module used for creation seems dangerous on this aspect.

================
Comment at: lib/LTO/LTOCodeGenerator.cpp:524
@@ -523,3 +523,3 @@
   // Add an appropriate DataLayout instance for this module...
-  mergedModule->setDataLayout(*TargetMach->getDataLayout());
+  mergedModule->setDataLayout(TargetMach->createDataLayout());
 
----------------
rafael wrote:
> I think this is from the days when we supported the module not having a DL. We should probably try to remove it in another patch.
I'm not very familiar with the JIT use case, but how is the client creating the module? And how can he makes sure that the DL on the module that he will give to the JIT is the one the TargetMachine expects? 
Is he supposed to keep a reference to the TargetMachine when creating the JIT?


http://reviews.llvm.org/D11103