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

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a subscriber: xur.
vsk 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());
----------------
tejohnson wrote:
> 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?
I'm not sure whether there's an advantage to scheduling splitting twice (as in, I haven't tested this out), but there's at least a compile-time argument for only doing it once. @xur would you be ok with committing your `bool LTOPreLink` change from D54175 early? Happy to send in patch for this if you'd prefer.


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

https://reviews.llvm.org/D58258