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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
