Re: [PATCH 0/6] Conversion of gimple types to C++ inheritance (v3)

[Jeff Law <law@redhat.com>]

On 10/31/13 10:26, David Malcolm wrote:
> The gimple statement types are currently implemented using a hand-coded
> C inheritance scheme, with a "union gimple_statement_d" holding the
> various possible structs for a statement.
>
> The following series of patches convert it to a C++ hierarchy, using the
> existing structs, eliminating the union. The "gimple" typedef changes
> from being a
>    (union gimple_statement_d *)
> to being a:
>    (struct gimple_statement_base *)
>
> There are no virtual functions in the new code: the sizes of the various
> structs are unchanged.
>
> It makes use of "is-a.h", using the as_a <T> template function to
> perform downcasts, which are checked (via gcc_checking_assert) in an
> ENABLE_CHECKING build, and are simple casts in an unchecked build,
> albeit it in an inlined function rather than a macro.
>
> For example, one can write:
>
>    gimple_statement_phi *phi =
>      as_a <gimple_statement_phi> (gsi_stmt (gsi));
>
> and then directly access the fields of the phi, as a phi.  The existing
> accessor functions in gimple.h become somewhat redundant in this
> scheme, but are preserved.
>
> The earlier versions of the patches made all of the types GTY((user))
> and provided hand-written implementations of the gc and pch marker
> routines.  In this new version we rely on the support for simple
> inheritance that I recently added to gengtype, by adding a "desc"
> to the GTY marking for the base class, and a "tag" to the marking
> for all of the concrete subclasses.  (I say "class", but all the types
> remain structs since their fields are all publicly accessible).
>
> As noted in the earlier patch, I believe this is a superior scheme to
> the C implementation:
>
>    * We can get closer to compile-time type-safety, checking the gimple
>      code once and downcasting with an as_a, then directly accessing
>      fields, rather than going through accessor functions that check
>      each time.  In some places we may want to replace a "gimple" with
>      a subclass e.g. phis are always of the phi subclass, to get full
>      compile-time type-safety.
>
>    * This scheme is likely to be easier for newbies to understand.
>
>    * Currently in gdb, dereferencing a gimple leads to screenfuls of text,
>      showing all the various union values.  With this, you get just the base
>      class, and can cast it to the appropriate subclass.
>
>    * With this, we're working directly with the language constructs,
>      rather than rolling our own, and thus other tools can better
>      understand the code. (e.g. doxygen).
>
> Again, as noted in the earlier patch series, the names of the structs
> are rather verbose.  I would prefer to also rename them all to eliminate
> the "_statement" component:
>    "gimple_statement_base" -> "gimple_base"
>    "gimple_statement_phi"  -> "gimple_phi"
>    "gimple_statement_omp"  -> "gimple_omp"
> etc, but I didn't do this to mimimize the patch size.  But if the core
> maintainers are up for that, I can redo the patch series with that
> change also, or do that as a followup.
>
> The patch is in 6 parts; all of them are needed together.
And that's part of the problem.  There's understandable resistance to 
(for example) the as_a casting.

There's a bit of natural tension between the desire to keep patches 
small and self-contained and the size/scope of the changes necessary to 
do any serious reorganization work.  This set swings too far in the 
latter direction :-)

Is there any way to go forward without the is_a/as_a stuff?  ie, is 
there're a simpler step towards where we're trying to go that allows 
most of this to go forward now rather than waiting?

>
>    * Patch 1 of 6: This patch adds inheritance to the various gimple
>      types, eliminating the initial baseclass fields, and eliminating the
>      union gimple_statement_d.   All the types remain structs.  They
>      become marked with GTY(()), gaining GSS_ tag values.
>
>    * Patch 2 of 6: This patch ports various accessor functions within
>      gimple.h to the new scheme.
>
>    * Patch 3 of 6: This patch is autogenerated by "refactor_gimple.py"
>      from https://github.com/davidmalcolm/gcc-refactoring-scripts
>      There is a test suite "test_refactor_gimple.py" which may give a
>      clearer idea of the changes that the script makes (and add
>      confidence that it's doing the right thing).
>      The patch converts code of the form:
>        {
>          GIMPLE_CHECK (gs, SOME_CODE);
>          gimple_subclass_get/set_some_field (gs, value);
>        }
>      to code of this form:
>        {
>          some_subclass *stmt = as_a <some_subclass> (gs);
>          stmt->some_field = value;
>        }
>      It also autogenerates specializations of
>          is_a_helper <T>::test
>      equivalent to a GIMPLE_CHECK() for use by is_a and as_a.
Conceptually I'm fine with #1-#3.

>
>    * Patch 4 of 6: This patch implement further specializations of
>      is_a_helper <T>::test, for gimple_has_ops and gimple_has_mem_ops.
Here's where I start to get more concerned.

>
>    * Patch 5 of 6: This patch does the rest of porting from union access
>      to subclass access (all the fiddly places that the script in patch 3
>      couldn't handle).
>
>    * Patch 6 of 6: This patch updates the gdb python pretty-printing
>      hook.
Conceptually #5 and #6 shouldn't be terribly controversial.

THe question is can we move forward without patch #4, even if that means 
we aren't getting the static typechecking we want?

Jeff