[PATCH] D20268: [wip] Resolution-based LTO API.

Peter Collingbourne via llvm-commits llvm-commits at lists.llvm.org
Tue Jun 14 18:25:21 PDT 2016 

pcc added inline comments.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:59
@@ +58,3 @@
+  /// pipeline. For example, a linker may use this function to implement
+  /// -save-temps, or to add its own resolved symbols to the module. If this
+  /// function returns false, any further processing for that task is aborted.
----------------
mehdi_amini wrote:
> "to add its own resolved symbols": so the linker will modify the module? (I notice the Module is not const in the hook prototype)
> What is the use-case?
Yes, it would modify the module.

The use case is to work around API deficiencies; the specific issue I had in mind was the special handling for common symbols required in the gold plugin (search for `addCommons`).

I wouldn't be surprised if we had to implement a similar workaround in libLTO as well, but I would not expect lld for example to need to modify the module, as we control both sides of the interface.

That said, the gold plugin only requires a mutable hook at one point (which happens to be post-internalize at the moment, but I think also pre-opt would work); maybe we should only allow a mutable hook at one of those points.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:62
@@ +61,3 @@
+  ///
+  /// Module hooks must be thread safe.
+  typedef std::function<bool(size_t Task, Module &)> ModuleHookFn;
----------------
mehdi_amini wrote:
> What level of "thread-safety" are we talking about? Is it just about the linker internal structure? Can the linker safely assume that only one thread will call one of these callbacks for a given Module? For a given LLVMContext?
Yes, this is with regard to the linker's internal structure.

On the LLVM side I think the right guarantee is a per-module guarantee (so the linker would be able to freely inspect and modify the provided module without having to worry about thread safety). Although of course at the moment this would be a per-context guarantee unless/until we made LLVMContext thread-safe.

I will change this comment to mention this.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:67
@@ +66,3 @@
+  /// (ThinLTO) the module, before modifying it.
+  ModuleHookFn PreOptModuleHook;
+
----------------
mehdi_amini wrote:
> What is the Task "ID" at this point? This would be a task that will not codegen in the case of a parallel backend?
> What is the Task "ID" at this point? 
I'm not sure what you are asking here. The task ID generally stays consistent across the whole pipeline.

> This would be a task that will not codegen in the case of a parallel backend?
Do you mean a distributed backend? In that case, none of the hooks will be called for ThinLTO tasks other than the combined module hook. I will document that.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:94
@@ +93,3 @@
+  typedef std::function<bool(ModuleSummaryIndex &Index)> CombinedIndexHookFn;
+  CombinedIndexHookFn CombinedIndexHook;
+};
----------------
mehdi_amini wrote:
> If the intent is just -save-temps, I'd rather have the Index passed as a const ref.
Makes sense, will do.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:107
@@ +106,3 @@
+/// completion, or false if a hook aborted the backend by returning false.
+bool backend(Config &C, StringRef TheTriple, const Target *TheTarget,
+             AddStreamFn AddStream, unsigned ParallelCodeGenParallelismLevel,
----------------
mehdi_amini wrote:
> Why the Triple string and Target here?
You're right, we don't need these. I'll change the implementation to use `Module::getTargetTriple()`.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:120
@@ +119,3 @@
+/// If GV's linkage is linkonce, change it to weak so that it won't be DCE'd.
+void upgradeLinkage(GlobalValue *GV);
+
----------------
mehdi_amini wrote:
> This is a strange API at this level. 
> And the naming is very generic for what the description says it does.
Yes, agree. After I finish the reorganization I mentioned earlier, I want to move this API to a library-internal header.

================
Comment at: include/llvm/CodeGen/LTOBackend.h:136
@@ +135,3 @@
+  DiagnosticHandlerFunction DiagHandler;
+};
+
----------------
mehdi_amini wrote:
> Why this `LTOLLVMContext` instead of a factory method in the Config class?
Because the `LTOLLVMContext` needs to own the `DiagHandler`.

================
Comment at: include/llvm/LTO/LTO.h:90
@@ +89,3 @@
+  comdat_alias = 1,
+  unable_to_find_target,
+};
----------------
mehdi_amini wrote:
> I think some doc may be nice for the enum values (I have no idea what is `comdat_alias` right now).
Will do.

================
Comment at: include/llvm/LTO/LTO.h:93
@@ +92,3 @@
+
+const std::error_category &lto_category();
+
----------------
mehdi_amini wrote:
> doc (public API in public header)
Will do

================
Comment at: include/llvm/LTO/LTO.h:292
@@ +291,3 @@
+  LTO(Config Conf, ThinBackend Backend = nullptr,
+      unsigned ParallelCodeGenParallelismLevel = 1);
+
----------------
mehdi_amini wrote:
> Not clear why `ParallelCodeGenParallelismLevel` is not part of `Config`.
> Also it does not apply to ThinLTO, which is not clear.
The idea was that `Config` controls everything except how code generation is "orchestrated" (that's why I'm passing in a `ThinBackend` here for example).

> Also it does not apply to ThinLTO, which is not clear.

I will document that more clearly.

================
Comment at: include/llvm/LTO/LTO.h:323
@@ +322,3 @@
+  ModuleSummaryIndex CombinedIndex;
+  StringMap<MemoryBufferRef> ModuleMap;
+
----------------
mehdi_amini wrote:
> There is too much state here. 
> I could see getting rid of almost *all* of these.
> Until you call `run()` on this class, I don't expect that we need any but the `Config` and `ModuleMap`.
I think the code is made more straightforward and easier to understand by incrementally building the combined module (for regular LTO) or the combined index (for ThinLTO) from `add()`, as you can see from the code exactly what happens for each module. That requires us to keep this state here.

Anyway, this state is an implementation detail, so it is not as important as the public API.

================
Comment at: include/llvm/LTO/LTO.h:385
@@ +384,3 @@
+
+  std::vector<std::unique_ptr<object::IRObjectFile>> ThinObjs;
+
----------------
mehdi_amini wrote:
> Why do we need this here?
Okay, since it looks like we're only using memory buffers here, we can probably just have an array of MemoryBuffers here (or maintain ModuleMap as a MapVector, or something).

================
Comment at: lib/CodeGen/LTOBackend.cpp:142-149
@@ +141,10 @@
+                  std::unique_ptr<Module> M) {
+  if (!opt(Conf, TheTriple, TheTarget, 0, *M, /*IsThinLto=*/false))
+    return false;
+
+  if (ParallelCodeGenParallelismLevel == 1)
+    return codegen(Conf, TheTriple, TheTarget, AddStream, 0, *M);
+  else
+    return splitCodeGen(Conf, TheTriple, TheTarget, AddStream,
+                        ParallelCodeGenParallelismLevel, std::move(M));
+}
----------------
lhames wrote:
> Apologies if this is a naive question - I'm still wrapping my head around this patch, but could opt/codegen/splitCodeGen be std::functions on Config rather than being plain functions that introspect the Config object to build the pass pipelines? This might allow you to decouple some of the pipeline setup logic from the LTO interface. It may also allow you to remove some of the hooks: E.g. If the user is supplying 'codegen', they can just add any module inspection code to their custom implementation, rather than having to set up 'PreCodeGenModuleHook'.
That's an interesting suggestion. My initial concern with that was that it bakes the required state for each pipeline phase into the API. For example, `codegen` requires a Triple and a Target, and many of the ThinLTO phases would require a combined module index, and those would need to be passed into the pipeline function via the "hook" in Config. If we ever change the required state, that would mean a lot of churn for each user of the API.

Although it occurred to me that we could avoid baking in the state like this:

```
// In LTOBackend.cpp
struct PromoteState {
  ModuleSummaryIndex &CombinedIndex;
};
struct CodeGenState {
  Config &C;
  StringRef TheTriple;
  const Target *TheTarget;
  AddStreamFn AddStream;
};

// In LTOBackend.h
struct PromoteState;
bool promote(size_t Task, Module &M, PromoteState &State);

struct CodeGenState;
bool codegen(size_t Task, Module &M, CodeGenState &State);

struct Config {
  // ...
  std::function<bool(size_t Task, Module &M, PromoteState &State)> Promote;
  std::function<bool(size_t Task, Module &M, CodeGenState &State)> CodeGen;
  // ...
};
```

that would still make the signatures of the Config "hooks" non-uniform, which would also make it more complicated to implement `save-temps`, which is about 90% of the point of these hooks. (Suppose I want to add save-temps to each pipeline phase, including "pre-optimization". I would not only need to write an individual wrapper function for each phase, but I would also need to know which phase comes "first" in order to run my pre-opt save-temps processing before it.) I would prefer to keep the hooks uniform unless there is a good reason not to do so.


http://reviews.llvm.org/D20268

More information about the llvm-commits mailing list