[PATCH] D124397: [Test + comment] Add a regression test to guard the 0 hot-caller threshold, and add comment near the threshold definition.

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 25 10:47:17 PDT 2022


tejohnson added a comment.

Thanks! Couple minor suggestions.



================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:716
   InlineParams IP = getInlineParamsFromOptLevel(Level);
+  // For PreLinkThinLTO + SamplePGO, set hot-caller threshold
+  // to 0 to disable hot callsite inline (as much as possible)
----------------
Nit here and in second instance of this comment: I think these lines are well under 80 chars? Can you change the line wrapping to make better use of the full line?


================
Comment at: llvm/lib/Passes/PassBuilderPipelines.cpp:721
+  // Note the cost of a function could be below zero
+  // (see test/Other/new-pm-thinlto-prelink-samplepgo-inline-threshold.ll).
   if (Phase == ThinOrFullLTOPhase::ThinLTOPreLink && PGOOpt &&
----------------
Not sure the test reference is needed or wanted here. I would remove that and if anything point to the place in the inline cost handling that shows how we can end up with a negative cost. But probably not necessary - in which case I would combine this with your earlier note "as much as possible" since I think this is the explanation for that aside? I.e. something like "as much as possible as the cost of a function could be negative". Or "as much as possible [1]" and later "[1] Note the cost of a function could be below zero."


================
Comment at: llvm/test/Other/new-pm-thinlto-prelink-samplepgo-inline-threshold.ll:7
+; `if.then` basic block is hot so the callsite threshold is set to 0.
+; `while.body.split` is basic block is cold so threshold is calculated using
+; the rest of default heuristics, which should be sufficient to inline a
----------------
I think first "is" should be removed (from "is basic block is cold").


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124397



More information about the llvm-commits mailing list