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

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/MemProfContextDisambiguation.cpp:2755
+  DenseSet<const ContextNode *> Visited;
+  for (auto &Entry : AllocationCallToContextNodeMap)
+    UpdateCalls(Entry.second, Visited, UpdateCalls);
----------------
tejohnson wrote:
> tejohnson wrote:
> > snehasish wrote:
> > > Add a comment for the work done in this DFS?
> > I believe it is this loop that is causing the unstable remarks ordering I'm seeing, which is emitted by the IR updating code invoked during the search. The key into this map is a pointer so we don't have a guaranteed stable ordering across runs. I could use a MapVector instead of a std::map, but rather than carry that extra space overhead throughout, probably it makes the most sense to build and then iterate a vector of all the allocation nodes (the map values) and sort by their OrigStackOrAllocId field, which is assigned in increasing order as they are inserted in the map and will be stable.
> I had to revert due to a different bot failure, in new test funcassigncloning.ll. This was in the expensive checks bot (which among other things also randomly shuffles elements before sorting). I built with expensive checks on but cannot reproduce the failure, even running thousands of times. 
> 
> However, looking at the excerpt of the failure the bot provided, I realized that the difference was a different ordering of some newly cloned nodes coming out of node cloning, which was not part of this patch. I realized it is almost certainly due to another iteration over AllocationCallToContextNodeMap, this time in identifyClones(), which would affect the ordering of the cloning. I decided it is probably safest to switch this map (as well as NonAllocationCallToContextNodeMap to avoid any problems there in the future) to MapVector.
> 
> Since this affects the prior patch, I will send a patch for just that change along with the part of funcassigncloning.ll that doesn't rely on this patch (including the checking which triggered the failure). I can then remove the change I made here to sort the allocation nodes.
That fix was made in D149924 which went in this morning, and the expensive checks bot succeeded with it. I'm going to rebase this one on top of that, remove the change made here to use a vector, incorporate some another bot fix I made earlier on top of this patch (to require asserts in the tests that use -stats), and will submit after retesting.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141077