[PATCH] D32783: [PartialInlining] Add frequency based cost analysis

[David Li via Phabricator via llvm-commits]

davidxl marked 10 inline comments as done.
davidxl added inline comments.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:66
+                             cl::Hidden, cl::ZeroOrMore,
+                             cl::desc("Relative frequency of outline region to "
+                                      "the entry block"));
----------------
eraman wrote:
> The description string and comment does not make it clear whether this is the upper limit (I think it is) or not.
Updated the comment.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:175
+  std::tuple<int, int, int>
+  computeOutliningCosts(Function *F, const FunctionOutliningInfo *OutliningInfo,
+                        Function *OutlinedFunction,
----------------
eraman wrote:
> Not sure if this has been clang-formatted because it looks like the next two lines can be merged into one (I may be wrong)
The ; will be @column 80 which clang-format does not like.


================
Comment at: lib/Transforms/IPO/PartialInlining.cpp:761
+  auto CalleeEntryCount = F->getEntryCount();
+  uint64_t CalleeEntryC = (CalleeEntryCount ? *CalleeEntryCount : 0);
+  bool AnyInline = false;
----------------
eraman wrote:
> Instead of using two variables with similar names, why not initialize the uint64_t to 0 above the previous if and inside the if do F->getEntryCount()->getValue() ? 
Both vars are used later. I changed the second var name to make it look friendiler.


https://reviews.llvm.org/D32783