[PATCH] D142747: [Pseudo Probe] Do not instrument EH blocks.
Wenlei He via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jan 30 11:12:48 PST 2023
wenlei accepted this revision.
wenlei added a comment.
This revision is now accepted and ready to land.
lgtm, thanks.
================
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:
> > 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.
> Actually just figured out that there are predecessors/successors API defined for MBB in MachineSSAContext.h. We can use that and `AccessorTraits` will not be needed at all.
>
>
Great - that's why I asked the first question. :)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D142747/new/
https://reviews.llvm.org/D142747
More information about the llvm-commits
mailing list