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

[hoy]

Summary

A context can be created by invoking the getFunctionProfileForContext function in two ways:

• by using a probe and its calling context.
• by removing the leaf frame from an existing contexts. The first way is used when generating a function profile for a given LBR range, and the input WasLeafInlined is computed depending on the actually probe in the LBR range. The second way is used when using the entry count of an inlinee function profile to update its inliner callsite count, so WasLeafInlined is unknown for the inliner frame.

The two invocations can happen in different order on different platforms, since the lbr ranges are stored in an unordered_map, and we are making sure ContextWasInlined is always set correctly.

This should fix the random test failure introduced by D121655

[hoy]

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

[wenlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

729

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?

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

729

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.

[wenlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

729

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?

[hoy]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

729

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)

[wenlei]

llvm/tools/llvm-profgen/ProfileGenerator.cpp

729

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.

[hoy]

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

llvm/tools/llvm-profgen/ProfileGenerator.cpp

if (I == ProfileMap.end()) {
     SampleContextFrames NewContext = *Contexts.insert(Context).first;
     SampleContext FContext(NewContext, RawContext);
     auto Ret = ProfileMap.emplace(FContext, FunctionSamples());
     if (WasLeafInlined)
       FContext.setAttribute(ContextWasInlined);
     FunctionSamples &FProfile = Ret.first->second;
     FProfile.setContext(FContext);
     return Ret.first->second;
+  } else {
+    // Update ContextWasInlined attribute for existing contexts.
+    // The current function can be called in two ways:
+    //  - when processing a probe of the current frame
+    //  - when processing the entry probe of an inlinee's frame, which
+    //    is then used to update the callsite count of the current frame.
+    // The two can happen in any order, hence here we are making sure
+    // `ContextWasInlined` is always set as expected.
+    // TODO: Note that the former does not always happen if no probes of the
+    // current frame has samples, and if the latter happens, we could lose the
+    // attribute. This should be fixed.
+    if (WasLeafInlined)
+      I->second.getContext().setAttribute(ContextWasInlined);
   }
+
   return I->second;
 }
 
 void CSProfileGenerator::generateProfile() {
   FunctionSamples::ProfileIsCSFlat = true;
 
   collectProfiledFunctions();