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

[Sergey Pupyrev via Phabricator via llvm-commits]

spupyrev marked 3 inline comments as done.
spupyrev added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfile.cpp:1561
     Instruction *MaxDestInst;
+    // Since profi treats multiple edges (multiway branches) as a single edge,
+    // we need to distribute the computed weight among the branches. We do
----------------
davidxl wrote:
> Is this a limitation? Can it be made to handle multi-graph?
The inference algorithm can process multi-graphs -- the questions is how do we store/process such edges outside of inference. The current implementation (see `SampleProfileLoaderBaseImpl::buildEdges`) merges all multiway branches into a single edge.
We could probably modify `buildEdges` and avoid merging, but that will change the existing non-profi functionality. An alternative is to apply this post-processing and keep the existing implementation as is.


================
Comment at: llvm/lib/Transforms/Utils/SampleProfileInference.cpp:75
+/// path and capacity scaling. The estimated time complexity is
+/// O(v(f)*m*log(n)), where m is the number of edges and v(f) is the value of
+/// the max flow.
----------------
rajeshwarv wrote:
> Could you please why the time complexity is not O(V*E^2) per http://go/wikip/Edmonds%E2%80%93Karp_algorithm and it is actually O(...) as described here?
> 
> Also, what is n? 
Unfortunately, it is not trivial to derive a tight worst-case estimation of our implementation, since we modify the classical algorithm (see in particular lines 206-209). For sure we're not doing more than v(f) augmentation iterations and each of them takes at most n*m steps. But I don't know if this worst-case bound can be achieved on some instance.

Furthermore, the worst-case complexity is a misleading concept to look into, since control-flow graphs have a "nice" structure. Typically the observed runtime is slightly super-linear.

I've updated the comment, feel free to suggest more explanations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109860