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

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/tools/llvm-profgen/ProfileGenerator.cpp:729
+    if (WasLeafInlined)
+      FProfile.getContext().setAttribute(ContextWasInlined);
   }
----------------
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)
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122844