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

[Hiroshi Inoue via Phabricator via llvm-commits]

inouehrs 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);
+
----------------
nemanjai wrote:
> 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).
For the default threshold, I modify to use 2000 (instead of 1000 in the previous patch) as the default value based on the _builtin_expect handling.


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


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


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:312
+  }
+  if (ColdBlock == NULL || ColdBlock->getSinglePredecessor() == NULL)
+    return NULL;
----------------
nemanjai wrote:
> 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.
I modified the code to set WeightHot and WeightCold only when it returns the non-null pointer as the return value.


================
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.
----------------
nemanjai wrote:
> Nit: please be careful with line length.
Fixed.


================
Comment at: lib/Transforms/Scalar/JumpThreading.cpp:325
+static void
+setMetaDataBasedOnColdBlock(BasicBlock* ColdBlock, unsigned WeightHot, unsigned WeightCold) {
+  SmallVector<BasicBlock *, 16> ColdBlocks;
----------------
nemanjai wrote:
> 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.
I added the comments. I use my simple implementation of dominance analysis because the CFG may be already modified if we have multiple branches with biased branch weights.
Also, a method with branch weight metadata is relatively rare and so I avoid requesting dominance analysis only for this purpose.


================
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);
----------------
nemanjai wrote:
> s/upword/upward
Oops. Fixed.


================
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);
----------------
nemanjai wrote:
> This formatting looks odd. I almost missed the `else`.
Fixed the style.


https://reviews.llvm.org/D31647