On Fri, 2020-01-10 at 10:24 -0700, Jeff Law wrote:
> On Wed, 2020-01-08 at 04:02 -0500, David Malcolm wrote:
> > Needs review.
> > 
> > Changed in v5:
> > - update ChangeLog path
> > - updated copyright years to include 2020
> > 
> > Changed in v4:
> > - Rework includes to avoid gcc-plugin.h
> > - Wrap everything with #if ENABLE_ANALYZER
> > - Replace auto_client_timevar with TV_ANALYZER_SUPERGRAPH
> > - Fix .dot output
> >   https://gcc.gnu.org/ml/gcc-patches/2019-11/msg02461.html
> > - Update for move of digraph.h
> > 
> > This patch adds a "supergraph" class that combines CFGs and
> > callgraph into
> > one directed graph, along with "supernode" and "superedge" classes.
> > 
> > gcc/analyzer/ChangeLog:
> >     * supergraph.cc: New file.
> >     * supergraph.h: New file.
> So in the back of my mind has always been how else can we use the
> infrastructure you're designing & implementing.  I'd always hoped the
> supergraph in particular would prove useful and it may.  But I just
> realized something as I was looking at the implementation.
> 
> It appears that you copy phis/statements from basic blocks into the
> supernodes.  That has important implications.  For example, we don't
> have mechanisms to keep the two views in sync.  So if we need the
> supergraph for something, we're probably going to need to rebuild it.
> 
> I don't think that implies we need to do anything now, just an
> important restriction we need to keep in mind if we want to re-use
> some
> of this stuff later.

Right - the analyzer pass builds a representation that's useful for it,
does a bunch of things to it, then tears it down; there's no
interaction with changes to the IR - but for the analyzer there doesn't
need to be.

> I don't see any EH handling mechansisms.  I realize we haven't
> focused
> on C++ and thus EH hasn't been the top of our minds, but are we going
> to have to handle that at some point?

Fair point.  The scope of the analyzer is C only right now, though I
think the only place I've documented that is in the internal docs,
where the Limitations section has "Only for C so far".  But I want to
support C++ in future releases.

> > diff --git a/gcc/analyzer/supergraph.h b/gcc/analyzer/supergraph.h
> > new file mode 100644
> > index 000000000000..81ea3ac08198
> > --- /dev/null
> > +++ b/gcc/analyzer/supergraph.h
> > @@ -0,0 +1,564 @@
> > +/* "Supergraph" classes that combine CFGs and callgraph into one
> > digraph.
> > +   Copyright (C) 2019-2020 Free Software Foundation, Inc.
> > +   Contributed by David Malcolm <dmalc...@redhat.com>.
> > +
> > +This file is part of GCC.
> > +
> > +GCC is free software; you can redistribute it and/or modify it
> > +under the terms of the GNU General Public License as published by
> > +the Free Software Foundation; either version 3, or (at your
> > option)
> > +any later version.
> > +
> > +GCC is distributed in the hope that it will be useful, but
> > +WITHOUT ANY WARRANTY; without even the implied warranty of
> > +MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> > +General Public License for more details.
> > +
> > +You should have received a copy of the GNU General Public License
> > +along with GCC; see the file COPYING3.  If not see
> > +<http://www.gnu.org/licenses/>;;.  */
> > +
> > +#ifndef GCC_ANALYZER_SUPERGRAPH_H
> > +#define GCC_ANALYZER_SUPERGRAPH_H
> > +
> > +#include "ordered-hash-map.h"
> > +#include "options.h"
> > +#include "cgraph.h"
> > +#include "function.h"
> > +#include "cfg.h"
> > +#include "basic-block.h"
> > +#include "gimple.h"
> > +#include "gimple-iterator.h"
> > +#include "digraph.h"
> Ugh.  Policy is to avoid doing this kind of thing.  Instead the
> includes are supposed to be in the .cc files.  

<rant>
FWIW, I've never understood the benefit of this policy; it feels like a
violation of the DRY principle, and I always find myself wondering
things like "what do I need to include before I can include gimple.h?".
</rant>

But time is short; I'll copy and paste the includes.

> This can be fixed
> without going through another review cycle.
> 
> OK with the #includes fixed.

Thanks; will do.

> jeff
> 

Reply via email to