This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 0/3] Compile-time gimple checking, without typedefs


On Fri, May 2, 2014 at 11:56 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> This patch series demonstrates a way of reimplementing the 89-patch series:
>   "[PATCH 00/89] Compile-time gimple-checking"
>      http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01148.html
>
> whilst avoiding introducing a pair of "gimple_foo/const_gimple_foo" typedefs
> for each subclass.
>
> It eliminates the "gimple" and "const_gimple" typedefs,
> renaming "gimple_statement_base" to "gimple_stmt", giving types:
>   "gimple_stmt *" and "const gimple_stmt *"
> thoughout the middle-end.  The rest of the gimple statement classes are
> renamed, converting the various
>   gimple_statement_with_FOO
> to:
>   gimple_stmt_with_FOO
> and the remainder:
>   gimple_statement_SOME_SUBCLASS
> to just:
>   gimple_SOME_SUBCLASS
>
> The idea is then to reimplement the earlier patch series, porting many of
> these:
>   gimple_stmt *something
> to point to some more concrete subclass; I've done this for GIMPLE_SWITCH.

Thanks for doing this.

> It requires two patches that I've already posted separately:
>
>   (A): "[PATCH] gengtype: Support explicit pointers in template arguments":
>           http://gcc.gnu.org/ml/gcc-patches/2014-05/msg00003.html
>        (which apparently will need reworking after wide-int is merged;
>         oh well).

I'll look at this after the wide-int merge (which hopefully happens soon...)

>   (B): "[PATCH 19/89] Const-correctness of gimple_call_builtin_p":
>           http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01194.html
>        (I have a separate bootstrap&regrtest in progress for just this one,
>         which appears to be pre-approved per Jeff's earlier comments).
>
> Of the 3 patches in the series itself:
>
>   Patch 1:
>     This one is handwritten: it renames the gimple statement classes in
>     their declarations, in the docs, in the selftests and renames explicit
>     uses of *subclasses*.
>
>   Patch 2:
>      This one is autogenerated: it does the mass conversion of "gimple" to
>      "gimple_stmt *" and "const_gimple" to "const gimple_stmt *" throughout
>      the code.
>
>      The conversion script is at:
>        https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/rename_gimple.py
>
>      It's not a real C++ parser, but it has some smarts for handling
>      awkward cases like:
>         gimple def0, def2;
>      which must become:
>         gimple_stmt *def0, *def2;
>                            ^ note the second '*' character.
>
>      You can see the selftests for the conversion script at:
>        https://github.com/davidmalcolm/gcc-refactoring-scripts/blob/master/test_rename_gimple.py
>
>   Patch 3:
>     This one is a port of the previously-posted:
>       "[PATCH 02/89] Introduce gimple_switch and use it in various places"
>          http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01154.html
>     to the new approach.  As per an earlier revision:
>       http://gcc.gnu.org/ml/gcc-patches/2014-04/msg01549.html
>     it *eliminates* the
>       stmt->as_a_gimple_switch ()
>     and
>       stmt->dyn_cast_gimple_switch ()
>     casting methods from the base class in favor of direct usage of is-a.h:
>       as_a <gimple_switch *> (stmt)
>     and
>       dyn_cast <gimple_switch *> (stmt)
>
>     This version of the patch also eliminates the
>        gimple_switch / const_gimple_switch
>     typedefs from the initial patch series in favor of "gimple_switch"
>     being the name of the subclass.

I wonder if, when we intoduce a gimple namespace, we can write

 dyn_cast <switch *> (stmt)

by means of ADL?  Thus, imagine just doing

namespace gimple {

 ... gimple.h contents ...
}

using gimple::gimple;
... a few more? ...

typedef gimple::gimple gimple;

And then

  gimple *stmt;
  gimple::switch *s = dyn_cast <switch *> (stmt);

(yeah, awkward 'switch' reserved keyword as you say below ...)

Thus, for all the new explicit sub-types you introduce use the namespace
syntax.  Can ADL work in our favors here with the way dyn_cast and
friends work?  Thus, do we really need to write dyn_cast
<gimple::switch *> (stmt)?

No, I'm not asking you to do that now (but a limited form of a gimple namespace
would be nice to have - using the "old-style" 'gimple' type avoids changing too
much code I suppose).

Just sth that you don't run out of work ;)

> Hopefully porting this first patch in the series gives a sense of what
> the end-result would look like.
>
> There are a few possible variations on the names we could pick in this
> approach.
>
> I deliberately renamed "gimple" to "gimple_stmt" since IIRC Andrew MacLeod
> had suggested something like this, for the potential of making "gimple"
> be a namespace. That said, using namespaces may be controversial, and
> would need further gengtype support, which may be tricky (i.e. fully teach
> gengtype about C++ namespaces, which may be a gengtype hack too far).
> [I'd much prefer to avoid C++ namespaces in the core code, mostly because
> of gengtype].
>
> Also, AIUI, Andrew is looking at introducing concepts of gimple types and
> gimple expressions, so "gimple" may no longer imply a *statement*.
>
> Alternatively, we could make the base class be just "gimple" (which would
> be more consistent with the names of the accessor functions).

I strongly prefer to name it 'gimple', not 'gimple_stmt'.  Because it's less
to type, and because it will make all other types shorter as well.  And because
'gimple' _is_ a stmt right now, so gimple_stmt is redundant.  Same applies
to gimple_stmt_with_FOO, just make it gimple_with_FOO.

I understand the namespace issue, but we don't have a namespace right now.
Also gimple::gimple works just fine, no?

> There's also the "bargain basement" namespaces approach, where we don't
> have an implicit "gimple" namespace, but just *pretend* we do, and rename
> the base type to "stmt", with e.g. "gimple_statement_phi" becoming just
> "phi". ["gimple_switch" would need to become "switch_", say, to avoid the
> reserved word].

Ick (for the 'switch' case ... CamelCase anyone? :)).

> Successfully bootstrapped and regrtested on x86_64-unknown-linux-gnu
> (Fedora 20) (on top of the 2 dependent patches mentioned above; equal
> results compared to a control build of r209953.
>
> How does this look? (for trunk; after 4.9.1 is released).

Not looking at the exact patches right now.

Thanks,
Richard.

>
> David Malcolm (3):
>   Handwritten part of conversion of "gimple" to "gimple_stmt *"
>   Autogenerated part of conversion of "gimple" to "gimple_stmt *"
>   Introduce gimple_switch and use it in various places
>
>  gcc/asan.c                               |   36 +-
>  gcc/builtins.c                           |   10 +-
>  gcc/builtins.h                           |    2 +-
>  gcc/c-family/c-gimplify.c                |    4 +-
>  gcc/calls.c                              |    2 +-
>  gcc/calls.h                              |    2 +-
>  gcc/cfgexpand.c                          |   48 +-
>  gcc/cfgexpand.h                          |    2 +-
>  gcc/cfgloop.c                            |    2 +-
>  gcc/cfgloop.h                            |    2 +-
>  gcc/cfgloopmanip.c                       |    4 +-
>  gcc/cgraph.c                             |   32 +-
>  gcc/cgraph.h                             |   24 +-
>  gcc/cgraphbuild.c                        |   12 +-
>  gcc/cgraphclones.c                       |    8 +-
>  gcc/cgraphunit.c                         |   12 +-
>  gcc/config/aarch64/aarch64-builtins.c    |    4 +-
>  gcc/config/alpha/alpha.c                 |   14 +-
>  gcc/config/i386/i386.c                   |   14 +-
>  gcc/config/rs6000/rs6000.c               |    4 +-
>  gcc/coretypes.h                          |   10 +-
>  gcc/cp/cp-gimplify.c                     |    2 +-
>  gcc/doc/gimple.texi                      |  824 ++++++++------
>  gcc/dumpfile.c                           |    4 +-
>  gcc/dumpfile.h                           |    4 +-
>  gcc/except.h                             |    2 +-
>  gcc/expr.c                               |   28 +-
>  gcc/expr.h                               |    2 +-
>  gcc/fold-const.c                         |    2 +-
>  gcc/fold-const.h                         |    2 +-
>  gcc/gdbhooks.py                          |    4 +-
>  gcc/ggc.h                                |    6 +-
>  gcc/gimple-builder.c                     |   26 +-
>  gcc/gimple-builder.h                     |   16 +-
>  gcc/gimple-fold.c                        |   58 +-
>  gcc/gimple-fold.h                        |    8 +-
>  gcc/gimple-iterator.c                    |   36 +-
>  gcc/gimple-iterator.h                    |   22 +-
>  gcc/gimple-low.c                         |   30 +-
>  gcc/gimple-low.h                         |    2 +-
>  gcc/gimple-pretty-print.c                |  110 +-
>  gcc/gimple-pretty-print.h                |   12 +-
>  gcc/gimple-ssa-isolate-paths.c           |   14 +-
>  gcc/gimple-ssa-strength-reduction.c      |   92 +-
>  gcc/gimple-ssa.h                         |   22 +-
>  gcc/gimple-streamer-in.c                 |   12 +-
>  gcc/gimple-streamer-out.c                |    8 +-
>  gcc/gimple-walk.c                        |   18 +-
>  gcc/gimple-walk.h                        |   12 +-
>  gcc/gimple.c                             |  315 +++---
>  gcc/gimple.h                             | 1782 +++++++++++++++---------------
>  gcc/gimplify-me.c                        |    4 +-
>  gcc/gimplify-me.h                        |    2 +-
>  gcc/gimplify.c                           |  102 +-
>  gcc/gimplify.h                           |   12 +-
>  gcc/graphite-poly.c                      |    8 +-
>  gcc/graphite-scop-detection.c            |   20 +-
>  gcc/graphite-sese-to-poly.c              |  168 +--
>  gcc/gsstruct.def                         |   52 +-
>  gcc/internal-fn.c                        |   40 +-
>  gcc/internal-fn.h                        |    2 +-
>  gcc/ipa-inline-analysis.c                |   45 +-
>  gcc/ipa-inline.c                         |    4 +-
>  gcc/ipa-profile.c                        |    2 +-
>  gcc/ipa-prop.c                           |   80 +-
>  gcc/ipa-prop.h                           |    6 +-
>  gcc/ipa-pure-const.c                     |   12 +-
>  gcc/ipa-ref.c                            |   10 +-
>  gcc/ipa-ref.h                            |   12 +-
>  gcc/ipa-split.c                          |   40 +-
>  gcc/java/java-gimplify.c                 |    2 +-
>  gcc/lto-streamer-in.c                    |   16 +-
>  gcc/lto-streamer-out.c                   |    6 +-
>  gcc/omp-low.c                            |  253 ++---
>  gcc/passes.c                             |    4 +-
>  gcc/predict.c                            |   32 +-
>  gcc/profile.c                            |    8 +-
>  gcc/sese.c                               |   18 +-
>  gcc/sese.h                               |    8 +-
>  gcc/ssa-iterators.h                      |   42 +-
>  gcc/stmt.c                               |    4 +-
>  gcc/system.h                             |    2 +-
>  gcc/target.def                           |    2 +-
>  gcc/testsuite/g++.dg/plugin/selfassign.c |    8 +-
>  gcc/testsuite/gcc.dg/plugin/selfassign.c |    8 +-
>  gcc/tracer.c                             |    4 +-
>  gcc/trans-mem.c                          |  114 +-
>  gcc/trans-mem.h                          |    2 +-
>  gcc/tree-affine.c                        |    2 +-
>  gcc/tree-call-cdce.c                     |   50 +-
>  gcc/tree-cfg.c                           |  277 ++---
>  gcc/tree-cfg.h                           |   22 +-
>  gcc/tree-cfgcleanup.c                    |   28 +-
>  gcc/tree-cfgcleanup.h                    |    2 +-
>  gcc/tree-chrec.c                         |   10 +-
>  gcc/tree-chrec.h                         |    6 +-
>  gcc/tree-complex.c                       |   34 +-
>  gcc/tree-core.h                          |    4 +-
>  gcc/tree-data-ref.c                      |   14 +-
>  gcc/tree-data-ref.h                      |    8 +-
>  gcc/tree-dfa.c                           |   16 +-
>  gcc/tree-dfa.h                           |    2 +-
>  gcc/tree-eh.c                            |  199 ++--
>  gcc/tree-eh.h                            |   38 +-
>  gcc/tree-emutls.c                        |    8 +-
>  gcc/tree-if-conv.c                       |   34 +-
>  gcc/tree-inline.c                        |  105 +-
>  gcc/tree-inline.h                        |    6 +-
>  gcc/tree-into-ssa.c                      |   62 +-
>  gcc/tree-into-ssa.h                      |    4 +-
>  gcc/tree-loop-distribution.c             |   67 +-
>  gcc/tree-nested.c                        |   32 +-
>  gcc/tree-nrv.c                           |    8 +-
>  gcc/tree-object-size.c                   |   28 +-
>  gcc/tree-outof-ssa.c                     |   26 +-
>  gcc/tree-outof-ssa.h                     |    4 +-
>  gcc/tree-parloops.c                      |   52 +-
>  gcc/tree-pass.h                          |    6 +-
>  gcc/tree-phinodes.c                      |   52 +-
>  gcc/tree-phinodes.h                      |   14 +-
>  gcc/tree-predcom.c                       |   62 +-
>  gcc/tree-profile.c                       |   32 +-
>  gcc/tree-scalar-evolution.c              |   72 +-
>  gcc/tree-scalar-evolution.h              |    2 +-
>  gcc/tree-sra.c                           |   62 +-
>  gcc/tree-ssa-alias.c                     |   38 +-
>  gcc/tree-ssa-alias.h                     |   14 +-
>  gcc/tree-ssa-ccp.c                       |   48 +-
>  gcc/tree-ssa-coalesce.c                  |   10 +-
>  gcc/tree-ssa-copy.c                      |   14 +-
>  gcc/tree-ssa-copyrename.c                |    2 +-
>  gcc/tree-ssa-dce.c                       |   40 +-
>  gcc/tree-ssa-dom.c                       |   90 +-
>  gcc/tree-ssa-dom.h                       |    2 +-
>  gcc/tree-ssa-dse.c                       |   10 +-
>  gcc/tree-ssa-forwprop.c                  |  154 +--
>  gcc/tree-ssa-ifcombine.c                 |   20 +-
>  gcc/tree-ssa-live.c                      |   18 +-
>  gcc/tree-ssa-loop-ch.c                   |    6 +-
>  gcc/tree-ssa-loop-im.c                   |   70 +-
>  gcc/tree-ssa-loop-ivcanon.c              |   20 +-
>  gcc/tree-ssa-loop-ivopts.c               |   68 +-
>  gcc/tree-ssa-loop-manip.c                |   32 +-
>  gcc/tree-ssa-loop-niter.c                |   56 +-
>  gcc/tree-ssa-loop-niter.h                |    4 +-
>  gcc/tree-ssa-loop-prefetch.c             |   16 +-
>  gcc/tree-ssa-loop-unswitch.c             |    8 +-
>  gcc/tree-ssa-loop.h                      |    2 +-
>  gcc/tree-ssa-math-opts.c                 |   90 +-
>  gcc/tree-ssa-operands.c                  |   42 +-
>  gcc/tree-ssa-operands.h                  |   10 +-
>  gcc/tree-ssa-phiopt.c                    |   86 +-
>  gcc/tree-ssa-phiprop.c                   |   18 +-
>  gcc/tree-ssa-pre.c                       |   52 +-
>  gcc/tree-ssa-propagate.c                 |   46 +-
>  gcc/tree-ssa-propagate.h                 |   14 +-
>  gcc/tree-ssa-reassoc.c                   |  176 +--
>  gcc/tree-ssa-sccvn.c                     |   58 +-
>  gcc/tree-ssa-sccvn.h                     |    8 +-
>  gcc/tree-ssa-sink.c                      |   22 +-
>  gcc/tree-ssa-strlen.c                    |   42 +-
>  gcc/tree-ssa-structalias.c               |   44 +-
>  gcc/tree-ssa-tail-merge.c                |   40 +-
>  gcc/tree-ssa-ter.c                       |   12 +-
>  gcc/tree-ssa-threadedge.c                |   46 +-
>  gcc/tree-ssa-threadedge.h                |    4 +-
>  gcc/tree-ssa-threadupdate.c              |   10 +-
>  gcc/tree-ssa-uncprop.c                   |   12 +-
>  gcc/tree-ssa-uninit.c                    |   78 +-
>  gcc/tree-ssa.c                           |   40 +-
>  gcc/tree-ssa.h                           |    4 +-
>  gcc/tree-ssanames.c                      |    8 +-
>  gcc/tree-ssanames.h                      |   16 +-
>  gcc/tree-stdarg.c                        |   12 +-
>  gcc/tree-switch-conversion.c             |   50 +-
>  gcc/tree-tailcall.c                      |   30 +-
>  gcc/tree-vect-data-refs.c                |  100 +-
>  gcc/tree-vect-generic.c                  |   22 +-
>  gcc/tree-vect-loop-manip.c               |   62 +-
>  gcc/tree-vect-loop.c                     |  174 +--
>  gcc/tree-vect-patterns.c                 |  194 ++--
>  gcc/tree-vect-slp.c                      |  124 +--
>  gcc/tree-vect-stmts.c                    |  244 ++--
>  gcc/tree-vectorizer.c                    |   22 +-
>  gcc/tree-vectorizer.h                    |  116 +-
>  gcc/tree-vrp.c                           |  152 +--
>  gcc/tree.c                               |    4 +-
>  gcc/tree.h                               |    4 +-
>  gcc/tsan.c                               |   12 +-
>  gcc/ubsan.c                              |   20 +-
>  gcc/value-prof.c                         |   96 +-
>  gcc/value-prof.h                         |   26 +-
>  gcc/vtable-verify.c                      |   10 +-
>  193 files changed, 4851 insertions(+), 4670 deletions(-)
>
> --
> 1.8.5.3
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]