[FS-AFDO] Assign discriminators to pseudo probes

[hoy]

Summary

This is the first change for FS-AFDO integration with CSSPGO. There are more patches coming.

With pseudo probes, we do not assign FS discriminators to any other instructions since we will be using only probes for profile correlation.

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids. Since they are not getting a FS discriminator, they will also be excluded from MIR profile loading. The corresponding changes will be in the subsequent patches.

[wenlei]

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

134–141

nit:

if ((ImprovedFSDiscriminator && I.isMetaInstruction()) || 
    (HasPseudoProbe && !I.isPseudoProbe()) {
  continue;
}

[hoy]

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

134–141

Actually we need to go with the probe check first because probe is also a meta instruction.

[wenlei]

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.

How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

134–141

Ok, but when HasPseudoProbe is false, are we still going to have probe instructions? I thought we won't.

[hoy]

In D147286#4242407, @wenlei wrote:

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.

How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?

They'll be handled separately. Basically they will be ignored in MIR sample loading.

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

134–141

Right, when HasPseudoProbe is false we don't have probes. But ImprovedFSDiscriminator and HasPseudoProbe can be true at the same time, so checking ImprovedFSDiscriminator && I.isMetaInstruction()) first will ignore all probe instructions.

[wenlei]

In D147286#4243608, @hoy wrote:

In D147286#4242407, @wenlei wrote:

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.

How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?

They'll be handled separately. Basically they will be ignored in MIR sample loading.

So you will need a separate patch to handle them, right? Without that, they are being ignored MIR sample loading. Perhaps worth clarifying in your change description.

My understand is: this is the first (of the two?) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch.

This is to reduce hash conflicts.

May be nitpicking but this can be confusing too. I thought it's not about conflict. When we have probe (i.e. CSSPGO is on), the anchors are probes instead of debug lines, so we just naturally assign FS discriminators on the probe (anchors) instead. Of course, since we have less probes than general instructions, hash collision is less likely to happen.

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

134–141

Ok, thanks for clarification. It's a bit tricky..

[hoy]

In D147286#4243840, @wenlei wrote:

In D147286#4243608, @hoy wrote:

In D147286#4242407, @wenlei wrote:

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.

How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?

They'll be handled separately. Basically they will be ignored in MIR sample loading.

So you will need a separate patch to handle them, right? Without that, they are being ignored MIR sample loading. Perhaps worth clarifying in your change description.

My understand is: this is the first (of the two?) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch.

Yes, there will be a separate change for probe integration with MIR sample loading and call probes will be handled there.

This is to reduce hash conflicts.

May be nitpicking but this can be confusing too. I thought it's not about conflict. When we have probe (i.e. CSSPGO is on), the anchors are probes instead of debug lines, so we just naturally assign FS discriminators on the probe (anchors) instead. Of course, since we have less probes than general instructions, hash collision is less likely to happen.

Right, we don't need to handle dwarf lines if we only want to use probe as the anchors. But handling probes and lines exclusively basically means we cannot do dual profiles for FS-AFDO. I think if hash conflict was not an issue we wouldn't go exclusive.

[wenlei]

In D147286#4243873, @hoy wrote:

In D147286#4243840, @wenlei wrote:

In D147286#4243608, @hoy wrote:

In D147286#4242407, @wenlei wrote:

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.

How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?

They'll be handled separately. Basically they will be ignored in MIR sample loading.

So you will need a separate patch to handle them, right? Without that, they are being ignored MIR sample loading. Perhaps worth clarifying in your change description.

My understand is: this is the first (of the two?) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch.

Yes, there will be a separate change for probe integration with MIR sample loading and call probes will be handled there.

Ok, please update the description to clarify. I would add something like "this is the first (of the two) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch."

This is to reduce hash conflicts.

May be nitpicking but this can be confusing too. I thought it's not about conflict. When we have probe (i.e. CSSPGO is on), the anchors are probes instead of debug lines, so we just naturally assign FS discriminators on the probe (anchors) instead. Of course, since we have less probes than general instructions, hash collision is less likely to happen.

Right, we don't need to handle dwarf lines if we only want to use probe as the anchors. But handling probes and lines exclusively basically means we cannot do dual profiles for FS-AFDO. I think if hash conflict was not an issue we wouldn't go exclusive.

I see what you mean now, I wasn't thinking about making them co-exist. Again, please make it clear in the change description so people understand it better.

Otherwise lgtm, thanks.

[hoy]

In D147286#4243883, @wenlei wrote:

In D147286#4243873, @hoy wrote:

In D147286#4243840, @wenlei wrote:

In D147286#4243608, @hoy wrote:

In D147286#4242407, @wenlei wrote:

Also call instructions are excluded since their dwarf discriminators are used for other purposes, i.e, storing probe ids.

How/where do we handle the FS discriminator for call probes? Or will that be handled in a separate patch?

They'll be handled separately. Basically they will be ignored in MIR sample loading.

So you will need a separate patch to handle them, right? Without that, they are being ignored MIR sample loading. Perhaps worth clarifying in your change description.

My understand is: this is the first (of the two?) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch.

Yes, there will be a separate change for probe integration with MIR sample loading and call probes will be handled there.

Ok, please update the description to clarify. I would add something like "this is the first (of the two) change for FS-AFDO integration with CSSPGO, where we handle non-call probes, and call probes will be handled in the 2nd patch."

This is to reduce hash conflicts.

May be nitpicking but this can be confusing too. I thought it's not about conflict. When we have probe (i.e. CSSPGO is on), the anchors are probes instead of debug lines, so we just naturally assign FS discriminators on the probe (anchors) instead. Of course, since we have less probes than general instructions, hash collision is less likely to happen.

Right, we don't need to handle dwarf lines if we only want to use probe as the anchors. But handling probes and lines exclusively basically means we cannot do dual profiles for FS-AFDO. I think if hash conflict was not an issue we wouldn't go exclusive.

I see what you mean now, I wasn't thinking about making them co-exist. Again, please make it clear in the change description so people understand it better.

Otherwise lgtm, thanks.

Sounds good, summary updated.

[wenlei]

On second thought, if we want AFDO and CSSPGO to co-exist when FS is on, we could assign discriminators independently for probe and non-probe instructions? That way there's no more collisions in each of the two category, and when loading profile, we ignore probe for AFDO, ignore non-probe for CSSPGO?

[hoy]

In D147286#4244544, @wenlei wrote:

On second thought, if we want AFDO and CSSPGO to co-exist when FS is on, we could assign discriminators independently for probe and non-probe instructions? That way there's no more collisions in each of the two category, and when loading profile, we ignore probe for AFDO, ignore non-probe for CSSPGO?

Yes, I thought about that too. We would need two set of maps, one for each.

The profile loading should naturally work as is with the IR sample loader. The loader knows what type of profiles it sees and uses lines or probes accordingly.

llvm/lib/CodeGen/MIRFSDiscriminator.cpp

 
 #include "llvm/CodeGen/MIRFSDiscriminator.h"
 #include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/Analysis/BlockFrequencyInfoImpl.h"
 #include "llvm/CodeGen/Passes.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/IR/Function.h"
+#include "llvm/IR/Module.h"
+#include "llvm/IR/PseudoProbe.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/raw_ostream.h"
 #include "llvm/Support/xxhash.h"
 #include "llvm/Transforms/Utils/SampleProfileLoaderBaseUtil.h"
 
 using namespace llvm;

 // Traverse the CFG and assign FD discriminators. If two instructions
 // have the same lineno and discriminator, but residing in different BBs,
 // the latter instruction will get a new discriminator value. The new
 // discriminator keeps the existing discriminator value but sets new bits
 // b/w LowBit and HighBit.
 bool MIRAddFSDiscriminators::runOnMachineFunction(MachineFunction &MF) {
   if (!EnableFSDiscriminator)
     return false;
-  if (!MF.getFunction().shouldEmitDebugInfoForProfiling())
+
+  bool HasPseudoProbe = MF.getFunction().getParent()->getNamedMetadata(
+      PseudoProbeDescMetadataName);
+
+  if (!HasPseudoProbe && !MF.getFunction().shouldEmitDebugInfoForProfiling())
     return false;
 
   bool Changed = false;
   using LocationDiscriminator =
       std::tuple<StringRef, unsigned, unsigned, uint64_t>;
   using BBSet = DenseSet<const MachineBasicBlock *>;
   using LocationDiscriminatorBBMap = DenseMap<LocationDiscriminator, BBSet>;
   using LocationDiscriminatorCurrPassMap =
bool MIRAddFSDiscriminators::runOnMachineFunction(MachineFunction &MF) {
   unsigned NumNewD = 0;
 
   LLVM_DEBUG(dbgs() << "MIRAddFSDiscriminators working on Func: "
                     << MF.getFunction().getName() << " Highbit=" << HighBit
                     << "\n");
 
   for (MachineBasicBlock &BB : MF) {
     for (MachineInstr &I : BB) {
-      if (ImprovedFSDiscriminator && I.isMetaInstruction()) {
+      if (HasPseudoProbe) {
+        // Only assign discriminators to pseudo probe instructions. Call
+        // instructions are excluded since their dwarf discriminators are used
+        // for other purposes, i.e, storing probe ids.
+        if (!I.isPseudoProbe())
+          continue;
+      } else if (ImprovedFSDiscriminator && I.isMetaInstruction()) {
         continue;
       }
       const DILocation *DIL = I.getDebugLoc().get();
       if (!DIL)
         continue;
-      unsigned LineNo = DIL->getLine();
+
+      // Use the id of pseudo probe to compute the discriminator.
+      unsigned LineNo =
+          I.isPseudoProbe() ? I.getOperand(1).getImm() : DIL->getLine();
       if (LineNo == 0)
         continue;
       unsigned Discriminator = DIL->getDiscriminator();
       uint64_t CallStackHashVal = 0;
       if (ImprovedFSDiscriminator)
         CallStackHashVal = getCallStackHash(DIL);
 
       LocationDiscriminator LD{DIL->getFilename(), LineNo, Discriminator,

llvm/test/CodeGen/X86/fsafdo_probe.ll

+; REQUIRES: x86_64-linux
+; REQUIRES: asserts
+; RUN: llc -enable-fs-discriminator -improved-fs-discriminator=false --debug-only=mirfs-discriminators  < %s  -o - 2>&1 | FileCheck %s --check-prefixes=V0
+; RUN: llc -enable-fs-discriminator -improved-fs-discriminator=true --debug-only=mirfs-discriminators  < %s  -o - 2>&1 | FileCheck %s --check-prefixes=V1
+
+; Check that fs-afdo discriminators are generated.
+; V0: foo.c:7:3: add FS discriminator, from 0 -> 11264
+; V0: foo.c:9:5: add FS discriminator, from 0 -> 11264
+; V0: Num of FS Discriminators: 2
+
+; V1: foo.c:7:3: add FS discriminator, from 0 -> 256
+; V1: foo.c:9:5: add FS discriminator, from 0 -> 256
+; V1: Num of FS Discriminators: 2
+
+target triple = "x86_64-unknown-linux-gnu"
+
+%struct.Node = type { ptr }
+
+define i32 @foo(ptr readonly %node, ptr readnone %root) !dbg !6  {
+entry:
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 1, i32 0, i64 -1), !dbg !8
+  %cmp = icmp eq ptr %node, %root, !dbg !8
+  br i1 %cmp, label %while.end4, label %while.cond1.preheader.lr.ph, !dbg !10
+
+while.cond1.preheader.lr.ph:
+  %tobool = icmp eq ptr %node, null
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 2, i32 0, i64 -1), !dbg !11
+  br i1 %tobool, label %while.cond1.preheader.us.preheader, label %while.body2.preheader, !dbg !11
+
+while.body2.preheader:
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 2, i32 0, i64 -1), !dbg !11
+  br label %while.body2, !dbg !11
+
+while.cond1.preheader.us.preheader:
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 3, i32 0, i64 -1), !dbg !10
+  br label %while.cond1.preheader.us, !dbg !10
+
+while.cond1.preheader.us:
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 3, i32 0, i64 -1), !dbg !10
+  br label %while.cond1.preheader.us, !dbg !10
+
+while.body2:
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 2, i32 0, i64 -1), !dbg !11
+  br label %while.body2, !dbg !11
+
+while.end4:
+  call void @llvm.pseudoprobe(i64 6699318081062747564, i64 4, i32 0, i64 -1), !dbg !12
+  ret i32 0, !dbg !12
+}
+
+declare void @llvm.pseudoprobe(i64, i64, i32, i64) #0
+
+attributes #0 = { inaccessiblememonly nocallback nofree nosync nounwind willreturn }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4}
+!llvm.pseudo_probe_desc = !{!13}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, isOptimized: true, debugInfoForProfiling: true, emissionKind: LineTablesOnly)
+!1 = !DIFile(filename: "foo.c", directory: "b/")
+!2 = !{}
+!3 = !{i32 2, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{}
+!6 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !7, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: true, unit: !0, retainedNodes: !2)
+!7 = !DISubroutineType(types: !2)
+!8 = !DILocation(line: 7, column: 15, scope: !9)
+!9 = !DILexicalBlockFile(scope: !6, file: !1, discriminator: 0)
+!10 = !DILocation(line: 7, column: 3, scope: !9)
+!11 = !DILocation(line: 9, column: 5, scope: !9)
+!12 = !DILocation(line: 14, column: 3, scope: !6)
+!13 = !{i64 6699318081062747564, i64 138464321060, !"foo"}