[PATCH] D159336: Statically analyze likely and unlikely blocks based on metadata

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:128
+
+  // [[unlikely]] and builtin_expect assigns 1/2000 as cold branch probability.
+  auto ColdProb = BranchProbability(1, std::min(2000, ColdBranchProbDenom.getValue()));
----------------
tejohnson wrote:
> Is there an internal option that determines the value used there that can be used in the code here? Looks like it is controlled by LikelyBranchWeight - can that be made a global option and referenced here so that they don't get out of sync?
found one. it has a higher probability but that is what other parts of llvm is using so i'll use the same here.


================
Comment at: llvm/test/Transforms/HotColdSplit/split-static-profile.ll:26
+
+; CHECK: internal void @foo.cold.1() #[[ATTR0:[0-9]+]]
+; CHECK-NEXT: newFuncRoot
----------------
tejohnson wrote:
> I think these cases should also be the same in the CHECK-PROB case - probably want a common check label to check the common cases between the two invocations, and a different check label just for the different parts.
> 
> Also, probably want to ensure that the hot calls/funcs are all above the cold calls/funcs - the checks below don't guarantee that.
makes sense. updated the tests.


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

https://reviews.llvm.org/D159336