[PATCH] D58258: [HotColdSplit] Schedule splitting late to fix perf regression

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Passes/PassBuilder.cpp:764
+  // is that this has a higher code size cost than splitting early.
+  if (EnableHotColdSplit && Phase != ThinLTOPhase::PreLink)
+    MPM.addPass(HotColdSplittingPass());
----------------
I see in the old PM that splitting is being moved post link for full LTO as well. IMO it should be in roughly the same place for the new PM. I guess the issue here is that there is no indication at this point that we are doing full LTO pre-link vs non-LTO. I would say either keep it pre-link in both PMs for full LTO or maybe better to pass down some indication that we are in the full LTO pre-link and not do it here in that case either (and then add it to buildLTODefaultPipeline as well to get it enabled for full LTO post-link). I'm reviewing another patch that needs to know this as well (D54175) and is adding a "bool LTOPreLink" parameter to this method. So maybe use the same thing?


================
Comment at: llvm/test/Other/opt-hot-cold-split.ll:4
 ; RUN: opt -mtriple=x86_64-- -Os -hot-cold-split=true -passes='thinlto-pre-link<Os>' -debug-pass-manager < %s -o /dev/null 2>&1 | FileCheck %s -check-prefix=THINLTO-PRELINK-Os
 ; RUN: opt -mtriple=x86_64-- -Os -hot-cold-split=true -passes='thinlto<Os>' -debug-pass-manager < %s -o /dev/null 2>&1 | FileCheck %s -check-prefix=THINLTO-POSTLINK-Os
 
----------------
Can you add a regular LTO postlink invocation as well? That would be --passes='lto<Os'


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

https://reviews.llvm.org/D58258