[PATCH] D104276: [CSSPGO][llvm-profgen] Ignore LBR records after interrup transition

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:460
+      // is likely due to interrupt which is usually unpaired. Ignore current
+      // and subsequent entries since they are likely from an unrelated
+      // pre-interrupt context.
----------------
hoy wrote:
> wenlei wrote:
> > What do you mean by unrelated pre-interrupt context? After interrupt is handled, execution is supposed to continue from the same spot. If we only missed one of the interrupt transition branch, the pre-interrupt IP should be right before the current IP.  If that's not the case, then what's missing is more than a transition branch from the pair - it then sounds like the problem is LBR buffer is not being cleared on context switch.
> Yes, LBR buffer is not cleared for all interrupts but task-scheduling interrupt. Interrupt does not reflect a normal execution flow. Pre-interrupt code and post-resume code should have the same context. For task-scheduling interrupt, the OS knows how to regroup them so that we have consistent execution flow in LBR for a single thread. Looks like it doesn't do that for other type of interrupts. We are working around here to break the LBR trace at the interrupt point.
> 
> On the second thought, we should also break at the resume point. 
> Yes, LBR buffer is not cleared for all interrupts but task-scheduling interrupt. 

I'm not sure if this is expected. My understanding of the expectation is that, since save/restore LBR states could be expensive, we don't need to do that for all interrupts and context switches. But the fall back is to clear LBR buffer, with that we shouldn't see inconsistent LBR entries. We never saw such inconsistent LBR on windows, and we also never saw that on linux until today. We need to follow up with kernel folks. 

> On the second thought, we should also break at the resume point. 

As a workaround, yes we need to bail out for both case. 


================
Comment at: llvm/tools/llvm-profgen/PerfReader.cpp:483
       IsArtificial = true;
     }
     // TODO: filter out buggy duplicate branches on Skylake
----------------
Would be clearer if we do this:

```
if (IsOutwards) {
 ...
}
else {
  // Not finding outward transition after seeing an inward transition ...
  if (PrevTrDst)
     break;
}
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104276