[PATCH] D109860: profi - a flow-based profile inference algorithm: Part I (out of 3)

[Hongtao Yu via Phabricator via llvm-commits]

hoy added a comment.

I just left a couple comments about minor issues. @davidxl  @xur @rajeshwarv We have done an around of code review internally. The current patch looks good to me in general.	Please let the author know if you have more comments. Thanks.



================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileInference.h:10
+/// \file
+/// This file provides the interface for the profile inference algorithm.
+//
----------------
nit: comment the `profi` abbreviation and its full name here? 


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:152
   void propagateWeights(FunctionT &F);
+  inline void applyProfi(Function &F, BlockEdgeMap &Successors,
+                         BlockWeightMap &SampleBlockWeights,
----------------
nit: no need to use the `inline` keyword.


================
Comment at: llvm/include/llvm/Transforms/Utils/SampleProfileLoaderBaseImpl.h:893
 
-    // Compute dominance and loop info needed for propagation.
-    computeDominanceAndLoopInfo(F);
+    if (!SampleProfileUseProfi) {
+        // Compute dominance and loop info needed for propagation.
----------------
davidxl wrote:
> It is probably better to restructure the code a little more.
> 
> Add a helper routine called "PrepareForPropagation(..)' or "InitForPropagation'. In this helper 
> 1) buildEdges;
> 2) compute equiv class for non-profi or profi specific initialization (basically move some code from propagateWeights' to here.
We still need this for MIR even if `SampleProfileUseProfi` is true. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109860