[PATCH] D96456: [ThinLTO, NewPM] Add Config::OptPassBuilderHook

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1066
                                          PassBuilder::OptimizationLevel Level) {
+    if (CodeGenOpts.OptimizationLevel == 0) {
+      if (!CodeGenOpts.ThinLTOIndexFile.empty())
----------------
Some comments around the cases being checked in this if-then-else would be helpful.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1328
+        CodeGenOpts.ThinLTOIndexFile.empty()) {
+      // This is testing distributed ThinLTO PostLink. O0 called optimized in
+      // PreLink.
----------------
I don't understand what you mean by "O0 called optimized"?
Also maybe make it clear that the first sentence goes with the second check?

Also, it isn't clear to me how this is preventing the sanitizers from being added in the ThinLTO pre-link compiles for optimized compiles, as shown in your tests. Unlike regular LTO pre-link optimized builds, which are still getting the sanitizers.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1541
+  Triple TargetTriple(M->getTargetTriple());
+  Conf.OptimizerLastPassBuilderHook = [&](PassBuilder &PB) {
+    addSanitizers(TargetTriple, CGOpts, LOpts, PB);
----------------
This will add sanitizers for the ThinLTO post-link compiles in a distributed build environment. Does something need to set these up for ThinLTO post-link compiles in the in-process ThinLTO case? In that case thinBackend() is invoked via LTO.cpp through the linker.


================
Comment at: llvm/include/llvm/LTO/Config.h:51
   std::vector<std::string> PassPlugins;
-  /// For adding passes that run right before codegen.
+  /// For adding passes that run by optimizer.
+  std::function<void(PassBuilder &)> OptimizerLastPassBuilderHook;
----------------
This one should say NewPM only.


================
Comment at: llvm/include/llvm/LTO/Config.h:53
+  std::function<void(PassBuilder &)> OptimizerLastPassBuilderHook;
+  /// For adding passes that run right before codegen (NewPM only).
   std::function<void(legacy::PassManager &)> PreCodeGenPassesHook;
----------------
This one should say legacy PM only


================
Comment at: llvm/include/llvm/LTO/Config.h:54
+  /// For adding passes that run by opt.
+  std::function<void(PassBuilder &)> OptPassBuilderHook;
   Optional<Reloc::Model> RelocModel = Reloc::PIC_;
----------------
vitalybuka wrote:
> aeubanks wrote:
> > vitalybuka wrote:
> > > tejohnson wrote:
> > > > Is this essentially the new PM equivalent to PreCodeGenPassesHook above (since this new hook runs at the end of opt)?
> > > > Also, might be good to name this like OptimizerLastPassBuilderHook or something like that to indicate its position within the opt pipeline.
> > > Kind of. PreCodeGenPassesHook is codegen and it's Legacy PM only.
> > I think the idea is that this callback adds PassBuilder callbacks in various parts of the the pipeline. Perhaps just `PassBuilderHook`? It's weird that we only allow one, but I guess we can make it a vector in the future if necessary.
> I guess the point of OptimizerLastPassBuilderHook name is to show that this is not any PassBuilder, but the one used in ::opt()
My point was just that they are effectively at the same place (the end of opt is pretty much the beginning of codegen), so I was wondering if it was worth making the legacy and new PM hooks be at the same place. But it probably doesn't matter much.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96456/new/

https://reviews.llvm.org/D96456