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

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Feb 14 16:56:04 PST 2019


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());
----------------
vsk wrote:
> 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.
I don't think we should do it twice, it should only be done once. The issue is that it is being added in different places in each PM with this patch. So my suggestion is to either move it post link here or pre link in the other PM.

@xur is out for a couple weeks, so if you want to add that part in the meantime that would be great!


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

https://reviews.llvm.org/D58258





More information about the llvm-commits mailing list