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

[Aditya Kumar via Phabricator via llvm-commits]

hiraditya marked 2 inline comments as done.
hiraditya added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:116
+
+  if (TrueWeight + FalseWeight == 0)
+    return;
----------------
tejohnson wrote:
> When does this happen?
i see this in https://llvm.org/doxygen/JumpThreading_8cpp_source.html so added as precaution. although i'm not 100% sure about it.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:119
+
+  if (BranchProbability(TrueWeight, TrueWeight + FalseWeight) <= BranchProbability(1, 1000))
+    AnnotatedColdBlocks.insert(CondBr->getSuccessor(0));
----------------
tejohnson wrote:
> Should this use BranchProbability::getBranchProbability instead of the constructor directly?
> 
> Also, is the (1,1000) threshold related to or the same as the branch probability assigned for builtin_expect and likely/unlikely?
builtins adds 1/2000 but i think that is too low. i've modified this to reflect builtins but added a flag for users to provide as they see fit.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:132
+  analyzeProfMetadata(BB, AnnotatedColdBlocks);
+  if (AnnotatedColdBlocks.count(&BB))
+    return true;
----------------
tejohnson wrote:
> I'm confused about how this works. It appears that analyzeProfMetadata analyzes the successors of BB, but then we query the information for the current BB. Ah, it looks like this works because this is called in a reversed PO traversal of the BBs?
> 
> It would be good to add some comments here. Otherwise it looks a little confusing as the back to back call and set lookup make it appear as though the call is doing the work for the set lookup in the same invocation, but really it is doing the work for a later invocation, with unstated traversal ordering assumptions.
yes it is due to RPOT. i'have added comments.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:624
     bool Cold = (BFI && PSI->isColdBlock(BB, BFI)) ||
-                (EnableStaticAnalysis && unlikelyExecuted(*BB));
+                (EnableStaticAnalysis && unlikelyExecuted(*BB, AnnotatedColdBlocks));
     if (!Cold)
----------------
tejohnson wrote:
> Should the static analysis i.e. the branch probabilities only be looked at if there is no BFI aka profile information? I think if there is profile info that the creation of the BFI/PSI has already taken expect and unlikely/likely attributes into account?
that is possible for unlikely/likely attributes true. but unlikelyExecuted does more analysis taking into account ehpad, noreturn functions etc. i'll refactor this code a little.


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

https://reviews.llvm.org/D159336