[PATCH] D96456: [ThinLTO, NewPM] Register sanitizers with OptimizerLastPassBuilderHook

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1069
+      if (!CodeGenOpts.ThinLTOIndexFile.empty()) {
+        // ThinLTOIndexFile is provideds so we must be in ThinLTO PostLink.
+        // For -O0 ThinLTO PreLink does basic optimization and triggers
----------------
nit: s/provideds/provided/
Also the line wrapping is off.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1070-1071
+        // ThinLTOIndexFile is provideds so we must be in ThinLTO PostLink.
+        // For -O0 ThinLTO PreLink does basic optimization and triggers
+        // OptimizerLastEPCallbacks. PostLink will not
+        // run optimizations and this callback should
----------------
vitalybuka wrote:
> aeubanks wrote:
> > We could fix this. It doesn't make sense for only -O0 to run OptimizerLastEPCallbacks prelink. Would that help simplify this?
> Do you mean move optimizer of -O0 into PostLink as well?
> Yes, this would be more consistent.
> Still I guess it should a separate patch.
> 
Or does it make sense to go the other way, i.e. find a way to add the sanitizer passes to the end of the pre-link of ThinLTO like for regular LTO? The ThinLTO pre-link doesn't execute the module optimization pipeline, but presumably at the end of buildThinLTOPreLinkDefaultPipeline we could invoke the OptimizerLastEPCallbacks? That avoids the issue of trying to get this working separately for in-process ThinLTO. And would be more consistent with regular LTO.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1079
+      if (CodeGenOpts.PrepareForThinLTO &&
+          CodeGenOpts.ThinLTOIndexFile.empty()) {
+        // We are here if we in ThinLTO PreLink.
----------------
You should only have to check PrepareForThinLTO to see if we are in the pre-link.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1081
+        // We are here if we in ThinLTO PreLink.
+        // For -O1 and above ThinLTO PreLink does no optimizations and do not
+        // triggers OptimizerLastEPCallbacks. See
----------------
nit: s/do not triggers/does not trigger/
Also the line wrapping is off.


================
Comment at: clang/lib/CodeGen/BackendUtil.cpp:1358
+      // and from the following buildO0DefaultPipeline here.
+      // We can't allow to instromen module twice and skip the second one.
+    } else {
----------------
nit: s/allow to instromen module/allow instrumenting the module/


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96456