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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:116
+
+  if (TrueWeight + FalseWeight == 0)
+    return;
----------------
When does this happen?


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:119
+
+  if (BranchProbability(TrueWeight, TrueWeight + FalseWeight) <= BranchProbability(1, 1000))
+    AnnotatedColdBlocks.insert(CondBr->getSuccessor(0));
----------------
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?


================
Comment at: llvm/lib/Transforms/IPO/HotColdSplitting.cpp:132
+  analyzeProfMetadata(BB, AnnotatedColdBlocks);
+  if (AnnotatedColdBlocks.count(&BB))
+    return true;
----------------
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.


================
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)
----------------
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?


================
Comment at: llvm/test/Transforms/HotColdSplit/split-static-profile.ll:34
+; CHECK: tail call noundef i32 @cold
+; attributes #[[ATTR0]] = { cold minsize }
+
----------------
Missing "CHECK:"?


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

https://reviews.llvm.org/D159336