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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:263
+    if (BB.isEHPad() || BB.isLandingPad()) {
+      Worklist.push(&BB);
+      EHBlocks.insert(&BB);
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/SampleProfileProbe.cpp:278-279
+      for (auto *Predecessor : predecessors(Successor)) {
+        if (Predecessor == Successor)
+          continue;
+        if (!EHBlocks.contains(Predecessor)) {
----------------
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. 


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747