[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);
----------------
hoy wrote:
> 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;
>     }
>   }
> ```
OK, by definition, `CatchSwitch` should refer to EH blocks only: https://llvm.org/docs/LangRef.html#catchswitch-instruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747