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

[Di Mo via Phabricator via llvm-commits]

modimo 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:
> 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
>Can a middle block of an EH region be reachable from non-EH path?
No, https://eel.is/c++draft/except.pre:
```
A handler will be invoked only by throwing an exception in code executed in the handler's try block or in functions called from the handler's try block.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747