[PATCH] D20268: Resolution-based LTO API.
Mehdi AMINI via llvm-commits
llvm-commits at lists.llvm.org
Thu Jul 14 13:55:03 PDT 2016
mehdi_amini added a comment.
Can you add one test for the .resolution.txt file with llvm-lto2?
================
Comment at: include/llvm/LTO/Config.h:82
@@ +81,3 @@
+ /// data structures. A module hook will never be called from multiple threads
+ /// with the same task ID, or the same module.
+ ///
----------------
"will never be called *concurrently* from multiple threads" -> I don't think we want to promise to pin a Module or a task to a given thread.
================
Comment at: include/llvm/LTO/Config.h:146
@@ +145,3 @@
+/// A derived class of LLVMContext that initializes itself according to a given
+/// Config object.
+struct LTOLLVMContext : LLVMContext {
----------------
You may add that the real reason to have a class here is that you don't want to tie the lifetime of the context to the configuration object and you need to keep a copy of the diagnostic handler alive.
Otherwise it may be easy for someone to nuke this anytime.
================
Comment at: include/llvm/LTO/LTO.h:103
@@ +102,3 @@
+ static ErrorOr<std::unique_ptr<InputFile>> create(MemoryBufferRef Object,
+ LTO &Lto);
+
----------------
IIUC the reason we need a context is because we can't get the information we want on an InputFile without a context for now.
Since we have plan to fix this (and this is acknowledged in the FIXME), I'd rather remove the LTO param now and add a Context inside the `InputFile` itself. It'll:
1) solve any multithreading question about parsing input files.
2) allow to easily get rid of this context later.
================
Comment at: include/llvm/LTO/LTO.h:321
@@ +320,3 @@
+ struct GlobalResolution {
+ /// The unmangled name of the global.
+ std::string IRName;
----------------
At least, can we wrap them in struct to separate this clearly:
```
struct {
unsigned ParallelCodeGenParallelismLevel;
LTOLLVMContext Ctx;
std::unique_ptr<Module> CombinedModule;
IRMover Mover;
} LTOState;
// These fields are for ThinLTO.
struct {
ThinBackend Backend;
ModuleSummaryIndex CombinedIndex;
MapVector<StringRef, MemoryBufferRef> ModuleMap;
DenseMap<GlobalValue::GUID, StringRef> PrevailingModuleForGUID;
} ThinLTOState;
```
It'll make the code more clear on violation (ThinLTO code accessing LTO state or vice-versa)
================
Comment at: lib/LTO/LTO.cpp:280
@@ +279,3 @@
+ CombinedModule->setTargetTriple(M.getTargetTriple());
+ CombinedModule->setDataLayout(M.getDataLayout());
+
----------------
This isn't clear to me "the client may want to add symbol definitions to it". Why doesn't the client add a regular LTO module if he needs to add its stuff?
================
Comment at: lib/LTO/LTOBackend.cpp:58
@@ +57,3 @@
+ PathPrefix = M.getModuleIdentifier();
+ std::string Path = PathPrefix + "." + PathSuffix + ".bc";
+ std::error_code EC;
----------------
How is this supposed to work with static archive? It'll write a bunch of file next to the input shared library?
Could we have a separate directory to stuff all these files with ThinLTO?
================
Comment at: lib/LTO/LTOBackend.cpp:149
@@ +148,3 @@
+ std::unique_ptr<TargetMachine> TM =
+ createTargetMachine(C, M.getTargetTriple(), TheTarget);
+ std::unique_ptr<raw_pwrite_stream> OS = AddStream(Task);
----------------
Nothing critical but we just already created a TM for the optimization pipeline.
================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:2
@@ +1,3 @@
+//===-- llvm-lto2: test harness for the resolution-based LTO interface ----===//
+//
+// The LLVM Compiler Infrastructure
----------------
Agree
================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:34
@@ +33,3 @@
+static cl::list<std::string> SymbolResolutions(
+ "r",
+ cl::desc("Specify a symbol resolution: filename,symbolname,resolution\n"
----------------
"r" is really not very explicit for an option, I think we usually have more self-explaining names.
================
Comment at: tools/llvm-lto2/llvm-lto2.cpp:44
@@ +43,3 @@
+ " visible outside of the LTO unit\n"
+ "A resolution for each symbol must be specified."),
+ cl::ZeroOrMore);
----------------
Are there incompatible combination? (I don't think so, but just checking).
https://reviews.llvm.org/D20268
More information about the llvm-commits
mailing list