[CSSPGO] Fix return value of getProbeWeight

[wlei]

Summary

Currently we didn't support multiple return type, we work around to use error_code to represent:

1. The dangling probe.
2. Ignore the weight of non-probe instruction

While merging the instructions' weight for the whole BB, it will filter out the error code. But If all instructions of the BB give error_code, the outside logic will mark it as a BB requiring the inference algorithm to infer its weight. This is different from the zero value which will be treated as a cold block.

Fix one place that if we can't find the FunctionSamples in the profile data which indicates the BB is cold, we choose to return zero.

Also refine the comments.

[wlei]

update comments

[hoy]

LGTM, thanks for the fix.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

572

Given it more thought, perhaps whether this is 0 or error_code is subject to something like profile-sample-accurate? I.e. with that flag, it means cold, otherwise it means unknown (error_code)?

The default behavior for getInstWeight the AutoFDO counterpart is returning error_code here. We don't have to do it now, but may be both AutoFDO and CSSPGO can check profile-sample-accurate here to decide between 0 or error_code? @wmi what do you think about AutoFDO?

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

572

Good point. Was thinking about this. When it comes here, Inst should be mostly from a function that has a profile, like

• If it is from a top-level inliner, the inliner should have a profile, otherwise it wouldn't be compiled.
• If it is from an inlinee, the inlinee should have a profile, otherwise it wouldn't be inlined (see SampleProfileLoader::getInlineCandidate).

[wmi]

llvm/lib/Transforms/IPO/SampleProfile.cpp

572

Currently profile-sample-accurate is only used on function profile, i.e., if a function profile is not present, and if profile-sample-accurate is true, compiler will set the function entry count to be zero. It is a balance between best performance/smaller binary size and performance stableness. In google currently some targets caring about size more than performance uses it since it will generate smaller binary.

If we do the same for instruction level profile, it will make the target using the flag to be more subject to source drift issue. I am not sure whether it will decrease the usability of the flag -- some targets currently using it can get their expected balance between binary size and performance stableness, and the balance may be broken if we introduce more performance unstableness to it.

[wenlei]

llvm/lib/Transforms/IPO/SampleProfile.cpp

572

I was hoping that by being more stringent on profile and trust zero counts to be zero, we would be able to save size and at the same time improve performance when profile is accurate.

From your experience, does profile-sample-accurate often lead to worse performance along with smaller size? If so, that is mostly because of source drift, right? If profile is refreshed very often, or used in instr. PGO like two step builds, it should not lead to worse performance..

I think we can leave it as is, or perhaps experiment with an additional switch for profile-sample-block-accurate for block level counts.. in order to avoid affecting existing usage.

[hoy]

llvm/lib/Transforms/IPO/SampleProfile.cpp

572

Yeah, profile-sample-block-accurate may work well for non-probe based profile. Currently for probe-based profile, newly added blocks may cause the entire function profile obsolete due to mismatched cfg checksum.

[wlei]

Update the comments according reviewers' discussion on profile-accurate. Currently, this version only work for probe-based code, it won't be affected by the source drift(see the comments)

[wenlei]

lgtm, thanks.

llvm/lib/Transforms/IPO/SampleProfile.cpp

572

Fair point on checksum - if we get here for csspgo, we don't expect source drift.

llvm/lib/Transforms/IPO/SampleProfile.cpp

ErrorOr<uint64_t> SampleProfileLoader::getInstWeight(const Instruction &Inst) {
   if (!ProfileIsCS)
     if (const auto *CB = dyn_cast<CallBase>(&Inst))
       if (!CB->isIndirectCall() && findCalleeFunctionSamples(*CB))
         return 0;
 
   return getInstWeightImpl(Inst);
 }
 
+// Here use error_code to represent: 1) The dangling probe. 2) Ignore the weight
+// of non-probe instruction. So if all instructions of the BB give error_code,
+// tell the inference algorithm to infer the BB weight.
 ErrorOr<uint64_t> SampleProfileLoader::getProbeWeight(const Instruction &Inst) {
   assert(FunctionSamples::ProfileIsProbeBased &&
          "Profile is not pseudo probe based");
   Optional<PseudoProbe> Probe = extractProbe(Inst);
+  // Ignore the non-probe instruction. If none of the instruction in the BB is
+  // probe, we choose to infer the BB's weight.
   if (!Probe)
     return std::error_code();
 
-  // Ignore danling probes since they are logically deleted and should not
-  // consume any profile samples.
+  // This is not the dangling probe from the training pass but generated by the
+  // current compilation. Ignore this since they are logically deleted and
+  // should not consume any profile samples.
   if (Probe->isDangling())
     return std::error_code();
 
   const FunctionSamples *FS = findFunctionSamples(Inst);
+  // If none of the instruction has FunctionSample, we choose to return zero
+  // value sample to indicate the BB is cold. This could happen when the
+  // instruction is from inlinee and no profile data is found.
+  // FIXME: This should not be affected by the source drift issue as 1) if the
+  // newly added function is top-level inliner, it won't match the CFG checksum
+  // in the function profile or 2) if it's the inlinee, the inlinee should have
+  // a profile, otherwise it wouldn't be inlined. For non-probe based profile,
+  // we can improve it by adding a switch for profile-sample-block-accurate for
+  // block level counts in the future.
   if (!FS)
-    return std::error_code();
+    return 0;
 
   // For non-CS profile, If a direct call/invoke instruction is inlined in
   // profile (findCalleeFunctionSamples returns non-empty result), but not
   // inlined here, it means that the inlined callsite has no sample, thus the
   // call instruction should have 0 count.
   // For CS profile, the callsite count of previously inlined callees is
   // populated with the entry count of the callees.
   if (!ProfileIsCS)