[PATCH] D121651: [llvm-profdata] Convert nested profile to CS flat profile.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:103
+  SPL_Nest = 0x1,
+  SPL_CS = 0x2,
+};
----------------
wenlei wrote:
> I think flat vs nest is better here (than cs vs nest).
Sounds good.


================
Comment at: llvm/include/llvm/ProfileData/SampleProf.h:375-376
+      CallTargets.erase(I);
+      if (*Count <= NumSamples)
+        NumSamples -= *Count;
+      else {
----------------
wenlei wrote:
> addCalledTarget does not add to NumSamples, but removeCalledTarget subtracts NumSamples. What's the reason for the inconsistency? 
I think. It's due to the implementation of the profile generator. Callsite block samples are computed by analyzing LBR ranges, and `NumSamples` is updated there, while call target samples are computed based on branch samples. D122609 provides an option to make them consistent.

`removeCalledTarget` handles both in one place. 


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:517
+    if (!PreserveContextAttributes)
+      ChildProfile->getContext().setAllAttributes(0);
+
----------------
wenlei wrote:
> nit: 0->ContextNone
Done.


================
Comment at: llvm/lib/ProfileData/SampleProf.cpp:529-530
+      // weight.
+      NodeProfile->removeCalledTargetAndUpdateTotalSamples(
+          ChildNode.CallSiteLoc.LineOffset, ChildNode.CallSiteLoc.Discriminator, OrigChildContext.getName());
     }
----------------
wenlei wrote:
> It looks like the discrepancy comes from the fact that flat profile has target counts included in total samples, but nest profile doesn't? Is this something that should be unified? I don't remember why we choose to include target counts (rather than SampleRecord::NumSamples) in total counts for flat profile. Was it intentional?
The profile generator, for both flat and nest profiles, only includes callsite counts in the total count but not target counts. Ideally they would be the same. Here we are excluding target counts from callsite counts, which in turn excluded from total counts.






Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121651