[PATCH] D31647: [JumpThreading] Propagate branch hint (biased branch weight) metadata

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:69
+          cl::desc("Threshold in branch weight metadata to identify a biased branch"),
+          cl::init(1000), cl::Hidden);
+
----------------
Maybe a comment mentioning why this default value (i.e. there's a bunch of mentions of 1:2000 identifying cold blocks, but this is set to 1000).


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:275
+  if (!MDS->getString().equals("branch_weights") ||
+      ProfMD->getNumOperands() != 3)
+    return false;
----------------
Please add an inline comment about the requirement for the MD having 3 operands.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:280
+  ConstantInt *CI2 = mdconst::extract<ConstantInt>(ProfMD->getOperand(2));
+  if (CI1 == NULL || CI2 == NULL)
+    return false;
----------------
Explicit comparisons against `NULL` seem at odds with other null checks added here.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:297
+                             uint64_t &WeightHot, uint64_t &WeightCold) {
+  BasicBlock* ColdBlock = NULL;
+  uint64_t WeightTrue, WeightFalse;
----------------
I think `nullptr` is a more common way to initialize a null pointer. Same goes for other uses of `NULL`.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:312
+  }
+  if (ColdBlock == NULL || ColdBlock->getSinglePredecessor() == NULL)
+    return NULL;
----------------
In general, I would recommend that a function with output parameters only set those output parameters on success. If there's a compelling reason to set `WeightHot` and `WeightCold` when returning `nullptr`, I think the comment at the top of the function should mention that behaviour.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:322
+/// If a cold block is known based on a branch hint, we first identify the cold region,
+/// i.e. the region post-dominated by the cold block. Then we set the branch hint for the
+/// conditional branches that jump into the cold region from non-cold region.
----------------
Nit: please be careful with line length.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:325
+static void
+setMetaDataBasedOnColdBlock(BasicBlock* ColdBlock, unsigned WeightHot, unsigned WeightCold) {
+  SmallVector<BasicBlock *, 16> ColdBlocks;
----------------
The logic in the function seems straightforward enough - looks like the cold region is all blocks dominated by `ColdBlock` and blocks post-dominated by `ColdBlock` (unless I'm misreading something). Then we set the branch weight on branches into that region.
Just seems like we should just be able to use the Dominators/PostDominators analyses for this kind of thing, but I very well may be missing something. In any case, if the existing analyses can't be used (even if it is an issue of analyses dependency) I think you should add a comment as to why that is.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1340
+      // we select the colder block as the destination to move the branch hint
+      // upword to the dominance frontier of this cold block.
+      if (WeightTrue < WeightFalse) MostPopularDest = TI->getSuccessor(0);
----------------
s/upword/upward


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:1341
+      // upword to the dominance frontier of this cold block.
+      if (WeightTrue < WeightFalse) MostPopularDest = TI->getSuccessor(0);
+      else                          MostPopularDest = TI->getSuccessor(1);
----------------
This formatting looks odd. I almost missed the `else`.


https://reviews.llvm.org/D31647