[PATCH] D20268: Resolution-based LTO API.

[Mehdi AMINI via llvm-commits]

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