[PATCH] D142747: [Pseudo Probe] Do not instrument EH blocks.

[Hongtao Yu via Phabricator via llvm-commits]

hoy added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:263
+    if (BB.isEHPad() || BB.isLandingPad()) {
+      Worklist.push(&BB);
+      EHBlocks.insert(&BB);
----------------
wenlei wrote:
> For this BFS to work, you want to start only from EH region entries. I think `isLandingPad` marks entries, but `isEHPad` could be in the middle of EH regions? IIUC, isEHPad is a superset of isLandingPad. 
> 
> Starting from middle will make it look like having non-EH predecessor, which is not true. It probably doesn't lead to wrong result in current implementation for two reasons: 1)   most likely lexical order guarantee EH entry is visited first, 2) if middle blocks are visited first and determined having non-EH predecessor, later visit from EH entry will still cover these blocks and mark them EH. But it feel unnecessary to start from the middle.
Yeah, EHPad is a superset of LandingPad, just checking against EHPad should be enough.

> Starting from middle will make it look like having non-EH predecessor, which is not true. 

Can a middle block of an EH region be reachable from non-EH path? I'm using it as seeds here. `isEHPad` is define below. I'm not sure how `CatchSwitch` is but all other types seem can be only run on EH paths?


```
  /// Return true if the instruction is a variety of EH-block.
  bool isEHPad() const {
    switch (getOpcode()) {
    case Instruction::CatchSwitch:
    case Instruction::CatchPad:
    case Instruction::CleanupPad:
    case Instruction::LandingPad:
      return true;
    default:
      return false;
    }
  }
```


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:278-279
+      for (auto *Predecessor : predecessors(Successor)) {
+        if (Predecessor == Successor)
+          continue;
+        if (!EHBlocks.contains(Predecessor)) {
----------------
wenlei wrote:
> Is this necessary? If Predecessor is non-EH, we will break out the loop later; if it's EH, line 274 should have skipped this Predecessor/Successor already. 
It's necessary if the successor forms a self-loop. In such case, the self back edge should be ignored.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:294
 void SampleProfileProber::computeProbeIdForBlocks() {
+  DenseSet<BasicBlock *> EHBlocks;
+  computeEHOnlyBlocks(EHBlocks);
----------------
wenlei wrote:
> nit: inclined to say something like `KnownColdBlocks`, and treating EH as one kind of code blocks. 
Sounds good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747