[PATCH] D125246: [CSSPGO][llvm-profgen] Reimplement CS profile generator using context trie

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:971-972
+      FunctionSamples *FSamples = Node->getFunctionSamples();
+      if (FSamples && !FSamples->getBodySamples().empty())
+        return true;
+
----------------
wlei wrote:
> wenlei wrote:
> > what makes function samples empty during conversion? 
> IMO, The std::move will move the `BodySamples` from the trie node's FSamples to the ProfileMap's FSamples. This is to check after the conversion no FSamples left  not moved.
The move-from object are in unspecified state according to standard, and we'd want to avoid accessing move-from objects. 


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:980-981
+  assert(
+      !CheckIfHasValidFSamples(&getRootContext()) &&
+      "After the conversion, there should be no valid FunctionSamples on trie");
+}
----------------
wlei wrote:
> wenlei wrote:
> > I was thinking about checking to make sure things are valid before conversion. Checking after conversion doesn't prevent double conversion where the 2nd conversion would be wrong. We could also have a simple state boolean to represent the statefulness and use that for checks.
> By a valid state, do you mean to check that the trie node's `getAllChildren` is not empty?
> 
I was thinking of valid state as an abstract term -- since the conversion destroy the original profile on trie through move etc, after conversion the function samples on the trie is no longer valid. We could simply have a boolean isProfileValidOnTrie as a state, set it to false inside convert function, and also check it is true at the beginning of conversion..  


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125246