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

[Wenlei He via Phabricator via llvm-commits]

wenlei added inline comments.


================
Comment at: llvm/include/llvm/Analysis/EHUtils.h:17
+/// Compute a list of blocks that are only reachable via EH paths.
+template <typename AccessorTraits, typename FunctionT, typename BlockT>
+static void computeEHOnlyBlocks(FunctionT &F, DenseSet<BlockT *> &EHBlocks) {
----------------
hoy wrote:
> wenlei wrote:
> > 1. Is the need for AccessorTraits because of the different API between MBB (member `predecessors()`) vs BB (standalone `predecessors()`)? 
> > 
> > 2. If we have to use AccessorTraits, can we avoid exposing it as template type at the top level API (`computeEHOnlyBlocks`)? I think we should be able to select the right traits based on BlockT. 
> > 
> > 3. Since we only need successor/predecessor, I'd suggest we decouple this from `afdo_detail::IRTraits`. That is, we define two small traits here for both MBB and BB.
> > Is the need for AccessorTraits because of the different API between MBB (member predecessors()) vs BB (standalone predecessors())?
> 
> Yes. I don't see code shared between BB and MBB often. I guess that's one reason preventing it from being shared easily.
> 
> > If we have to use AccessorTraits, can we avoid exposing it as template type at the top level API (computeEHOnlyBlocks)? I think we should be able to select the right traits based on BlockT
> 
> Can you elaborate a little more? Iike based on typeid check?
> 
> > Since we only need successor/predecessor, I'd suggest we decouple this from afdo_detail::IRTraits. That is, we define two small traits here for both MBB and BB.
> 
> Makes sense.
> 
> Can you elaborate a little more? Iike based on typeid check? 

I haven't looked at how. But I suppose with some investigation we should be able to find a way to hide the traits from API. We have precedence - there's not any need for specifying the traits when instantiating a sample loader. 


================
Comment at: llvm/lib/CodeGen/MachineFunctionSplitter.cpp:90
+static void setDescendantEHBlocksCold(MachineFunction &MF) {
+  struct MIRTraits {
+    static auto getPredecessors(MachineBasicBlock *BB) {
----------------
hoy wrote:
> wenlei wrote:
> > As mentioned above, this better be moved into `EHUtils.h`.
> I thought about that, but I ended up defining them here because otherwise it'll introduce a cyclic dependency between Analysis and Codegen.
Ok, I see. I guess that's why `IRTraits<MachineBasicBlock>` for sample loader is also in codegen. 

This is not ideal - the traits is supposed to be part of the infra/framework, but now you're defining it at the client side. This is also worse than the MIRSampleProfile case because one can argue it's still part of framework there. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142747