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

[Diego Novillo via llvm-commits]

dnovillo added a comment.

Apologies for the delay, Dehao.  Thanks for the description.  That needs to be added to the code, so we have it for future reference.  I suspect that several of the spots where I was confused and asked for comments are going to be good candidates to sprinkle that high-level description around.

Thanks.


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

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:472
@@ +471,3 @@
+  if (CI && findCalleeFunctionSamples(*CI))
+    return 0; 
+
----------------
And this is because we will redo that inlining decision later, right?  What about the inlining decisions we decide not to redo?

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:807
@@ -800,1 +806,3 @@
 /// \param F  Function to process.
+/// \param UpdateBlockCount  Whether we could update basic block counts that has
+///                          already been annotated.
----------------
s/could/should/ here, right?

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:904
@@ -894,1 +903,3 @@
             EdgeWeights[UnknownEdge] = 0;
+          const BasicBlock *OTHER_EC;
+          if (i == 0)
----------------
Convention is camel case for locals => OtherEC

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:916
@@ -899,1 +915,3 @@
         }
+      } else if (VisitedBlocks.count(EC) && BlockWeights[EC] == 0) {
+        if (i == 0) {
----------------
Comment here, please.  What is this doing?

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1008
@@ +1007,3 @@
+
+  for (auto &BI : F) {
+    BasicBlock *BB = &BI;
----------------
Comment here, please.  What is this doing?

================
Comment at: lib/Transforms/IPO/SampleProfile.cpp:1031
@@ +1030,3 @@
+  }
+
+  VisitedEdges.clear();
----------------
The block below also needs some commenting.  Why the multiple stages of propagation?  Can this be re-factored a bit?


https://reviews.llvm.org/D23224