[PATCH] D122844: [llvm-profgen] Fixing a context attribure update issue due to a non-derministic processing order on different platforms.

[Wenlei He via Phabricator via llvm-commits]

wenlei accepted this revision.
wenlei added inline comments.
This revision is now accepted and ready to land.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:729
+    if (WasLeafInlined)
+      FProfile.getContext().setAttribute(ContextWasInlined);
   }
----------------
hoy wrote:
> wenlei wrote:
> > hoy wrote:
> > > wenlei wrote:
> > > > This fix assumes that the 1st is always going to be called for given context, and it's just a matter of order. Is that actually true?
> > > > 
> > > > What is the exact call path for both cases?  
> > > Yes. Since we go through the lbr samples in an unorderd way (see CSProfileGenerator::populateBodySamplesWithProbes, `ProbeCounter` is an unordered_map), #1 and #2 can be called in arbitrary order on different platforms. If #1 is called before #2, the context will have the correct attribute set even without this fix. On the contrary, we need the fix for #1 to update already existing context.
> > Sorry if the question wasn't clear, and not sure if you answered the question:
> > 
> >  - 1st is always going to be called for given context? asking because if in some cases we only call 2nd, then the flag will be missing despite being deterministic. 
> >  - What is the exact call path for both cases?
> > 1st is always going to be called for given context?
> 
> That's not always true. The context could never be executed but its inlinee context can be. Yes, the flag could be missing in this case. 
> 
> 
> > What is the exact call path for both cases?
> 
> 
> For #1, when the inliner frame has a probe executed. Something like
> 
> 
> ```
> llvm::sampleprof::CSProfileGenerator::getFunctionProfileForContext(llvm::SmallVector<llvm::sampleprof::SampleContextFrame, 1u> const&, bool) (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:710)
> llvm::sampleprof::CSProfileGenerator::getFunctionProfileForLeafProbe(llvm::ArrayRef<llvm::sampleprof::SampleContextFrame>, llvm::MCDecodedPseudoProbe const*) (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:1131)
> llvm::sampleprof::CSProfileGenerator::populateBodySamplesWithProbes(std::map<std::pair<unsigned long, unsigned long>, unsigned long, std::less<std::pair<unsigned long, unsigned long> >, std::allocator<std::pair<std::pair<unsigned long, unsigned long> const, unsigned long> > > const&, llvm::ArrayRef<llvm::sampleprof::SampleContextFrame>) (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:1041)
> llvm::sampleprof::CSProfileGenerator::generateProbeBasedProfile() (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:1018)
> llvm::sampleprof::CSProfileGenerator::generateProfile() (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:729)
> main (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/llvm-profgen.cpp:183)
> 
> ```
> 
> For #2, when an inlinee's probe is executed and the probe is an entry probe. The entry probe count is then used to update the inliner's callsite count.
> 
> ```
> 
> llvm::sampleprof::CSProfileGenerator::getFunctionProfileForContext(llvm::SmallVector<llvm::sampleprof::SampleContextFrame, 1u> const&, bool) (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:728)
> llvm::sampleprof::CSProfileGenerator::populateBodySamplesWithProbes(std::map<std::pair<unsigned long, unsigned long>, unsigned long, std::less<std::pair<unsigned long, unsigned long> >, std::allocator<std::pair<std::pair<unsigned long, unsigned long> const, unsigned long> > > const&, llvm::ArrayRef<llvm::sampleprof::SampleContextFrame>) (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/**ProfileGenerator.cpp:1081**)
> llvm::sampleprof::CSProfileGenerator::generateProbeBasedProfile() (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:1034)
> llvm::sampleprof::CSProfileGenerator::generateProfile() (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/ProfileGenerator.cpp:745)
> main (/home/hoy/src/llvm-project/llvm/tools/llvm-profgen/llvm-profgen.cpp:183)
> __libc_start_main (:61)
> _start (:13)
> ```
Ok, please point out the answer to my 1st question in the comment, and mention the current fix is a hack (with a FIXME or TODO), to be fixed later. Please also try to make the rest of the comment more concise.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122844