[PATCH] D23224: Fine tuning of sample profile propagation algorithm.

[Dehao Chen via llvm-commits]

danielcdh added inline comments.

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:463
@@ -462,4 +462,3 @@
 
-  // Ignore all dbg_value intrinsics.
-  const IntrinsicInst *II = dyn_cast<IntrinsicInst>(&Inst);
-  if (II && II->getIntrinsicID() == Intrinsic::dbg_value)
+  // Ignore all intrinsics and branch instructions.
+  if (isa<BranchInst>(Inst) || isa<IntrinsicInst>(Inst))
----------------
dnovillo wrote:
> Why are we ignoring branches? IS it because they don't matter for weight calculation?  Should we return 0 for them, or simply ignore them?
Because branch instruction's debug info are usually attributed to sources outside the basic block. So we simply ignore all branches when annotating.

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:472
@@ +471,3 @@
+  if (CI && findCalleeFunctionSamples(*CI))
+    return 0; 
+
----------------
dnovillo wrote:
> And this is because we will redo that inlining decision later, right?  What about the inlining decisions we decide not to redo?
At this point, all necessary inlining has been redone. If we decided not to redo it, it means it's hot, then we will mark it's count as 0 to prevent from getting inlined in later inlining phase.

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1031
@@ +1030,3 @@
+  }
+
+  VisitedEdges.clear();
----------------
dnovillo wrote:
> The block below also needs some commenting.  Why the multiple stages of propagation?  Can this be re-factored a bit?
Comments added. I think this just calls the same function 3 times, not sure if using a loop to do it three times would simplify the code much.


https://reviews.llvm.org/D23224