[PATCH] D141077: [MemProf] Context disambiguation cloning pass [patch 3/4]

[Teresa Johnson via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2309
+template <typename DerivedCCG, typename FuncTy, typename CallTy>
+bool CallsiteContextGraph<DerivedCCG, FuncTy, CallTy>::assignFunctions() {
+  bool Changed = false;
----------------
snehasish wrote:
> Thanks for the detailed inline comments. Can we summarize the heuristic as pseudo-code in a function comment above the implementation too? 
I wasn't sure how much detail you wanted here. PTAL and let me know what I added is helpful.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2364
+      // Keep track of the clones of callsite Node that need to be assigned to
+      // function clones. This list may be expanded in the loop body below if we
+      // find additional cloning is required.
----------------
snehasish wrote:
> [optional] This sounds like we are implementing a worklist. The loop below uses indices and conditions based on them, I wonder if it would be clearer to restructure as a traditional worklist e.g. `while(!ClonesWorklist.empty()) { ...}`. I think some of the conditions inside can be rewritten and if the resulting code would be easier to follow. Not a major concern but wanted to get your opinion.
Good idea, switched to a worklist approach. The uses of "I" below are switched to uses of a new variable NodeCloneCount that effectively counts from 1, so the uses are changed slightly.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2385
+          // If this is the first callsite copy, assign to original function.
+          if (I == 0) {
+            // Since FuncClonesToCallMap is empty in this case, no clones have
----------------
snehasish wrote:
> Can we hoist the contents of this if condition outside the loop? Then we can start the iteration at index 1 and remove this check.
No, because we don't always get here for I==0. We would only get here for the I==0 case if the FuncClonesToCallMap is empty, i.e. this callsite's function has not been cloned previously. The function might have already been cloned if we previously processed a different callsite from the same function, in which case we will handle this clone like any other clone in the later code that assigns it to an existing function version (which will be the original copy in practice, but we utilize the same code to do this as for all other clones).


================
Comment at: llvm/test/ThinLTO/X86/memprof-basic.ll:109
 
+attributes #0 = { noinline optnone }
+
----------------
snehasish wrote:
> I don't the addition of these attributes is necessary for this patch. Is this left over from the split or just added to make the test more resilient? Perhaps split them out and commit as NFC for the latter.
Ah, this is leftover from the split. It is needed for the IR checking. I will remove this change from here (and from other tests) and eventually they will show up in patch 4 when I rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141077