Re: [RFC] Compile-time gimple-checking (again)

[Richard Biener <richard dot guenther at gmail dot com>]

On Wed, Oct 15, 2014 at 6:15 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> Back in March I posted an 89-patch kit to expand and make use of the
> gimple statement class hierarchy to move much of the  type-checking of
> statement accessors to be at compile-time rather than run-time:
>   https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01148.html
>
> I'd like to get these patches into trunk in some form before stage 1
> closes.
>
> I'll attempt to summarize the earlier discussion about these patches;
> please forgive me if I'm mischaracterizing things.
>
> There was some discussion about what the resulting classes and API
> should look like.
>
> Jeff reviewed the patches and approved them (modulo some issues that
> I've fixed), conditional on resolving the API design issues that arose
> in the discussion - and on holding off until 4.9.1 was released.
>
> Richi wanted me to change "gimple" to be the base class, rather than
> being a typedef of a *pointer* to the base class:
>    https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01520.html
> thus avoiding numerous typedefs for all of the subclasses, in their
> const and non-const variants. i.e. the "pointerness" of the type becomes
> explicit; everywhere we currently have a:
>   gimple stmt;
> that's implicitly a ptr, we would instead have:
>   gimple *stmt;
> making the pointer explicit.
>
> After some discussion about whether we wanted to keep the "gimple_"
> prefix for the various subclasses , I posted an email with various ideas
> as to what the API could look like:
>   * Status quo
>   * The April 2014 patch series (with indirect use of is-a.h)
>   * Direct use of is-a.h, retaining typedefs of pointers
>   * Explicit pointers, rather than typedefs
>   * Implicit naming
>   * Namespaces (explicit)
>   * Namespaces (implicit)
>   * C++ references (without namespaces)
>   * C++ references (with implicit namespaces)
> See https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01562.html for the
> full examples.
>
> There was a followup discussion about whether we should convert the
> accessors to be *methods* rather than functions, but Richi felt that was
> a step too far at this time:
> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg01824.html
>
> Jeff asked Richi (in
> https://gcc.gnu.org/ml/gcc-patches/2014-04/msg02082.html ):
>>> Anyway, gazillion new typedefs are ugly :/  (typedefs are ugly)
>> Yea, can't argue with that. However, do we want to ask David to fix up
>> the gimple vs gimple * vs const gimple * vs const gimple & as a
>> prerequisite for this patchset or are you comfortable going forward
>> with the patchset, then researching if there's a cleaner way to handle
>> the const/typedef issues?
>
> Richi responded (in
> https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00064.html):
>> Well, I'd like to see both and one affects the other. Doing the
>> const correctness thing first seems more natural to me.
>> Of course both need to wait for 4.9.1.
>
> I posted a possible renaming of the gimple subclasses as:
>   https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00128.html
> incorporating a mass-renaming, and changing the "pointerness" of the
> gimple type to be explicit rather than implicit.
>
> After some discussion about what the types should be named Jakub
> suggested a simple "g" prefix:
>   https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00248.html
>
> giving a class hierarchy like this:
>   https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00346.html
>
> Jeff agreed:
>   https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00347.html
>
> Richi agreed:
>   https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00349.html
>
> (did I correctly summarize things?)

Yes.

> I first attempted to implement this by doing the "pointerness"
> conversion first, to avoid lots of const_ typedefs, then updating the
> other patches. i.e. a big autogenerated patch, followed by 89
> handwritten patches.
>
> I only managed about 1/3 of the kit, and what I had bitrotted very
> quickly.
> Based on that, I don't think it's workable to do the big automated
> gimple -> gimple * conversion upfront.
>
> I think two workable approaches are:
> (i) save any big automated conversions until last
> (ii) do an automated conversion on the .patch files themselves
>
> I've updated the patch series; they can be seen at:
> https://dmalcolm.fedorapeople.org/gcc/patch-backups/gimple-classes/v11-patches/
>
> This contains the patches from before.
>
> I've rebased them against r216157 from Monday (2014-10-13) aka
> fc222f445c6108418196a1b48703d350f3c3d45a.
>
> This required numerous essentially mechanical changes to the patches
> e.g. for the big reworking of cgraph functions to be methods.  I've been
> working on the assumption that these various changes aren't going to
> require a re-review.
>
> I also removed the unloved as_a/dyn_cast methods from the gimple base
> class in favor of as_a<>/dyn_cast<> functions from is-a.h
>
> I've successfully bootstrapped and regtested the cumulative effect of
> the patchkit on Fedora 20 x86_64.
>
> Some questions:
>
> Are people still happy with the proposed naming from:
>   https://gcc.gnu.org/ml/gcc-patches/2014-05/msg00346.html
> (incorporating the "pointerness" change of gimple to require "gimple
> *").
>
> How should I proceed?  The proposal is going to require one or more
> auto-generated "megapatches", and such a patch is going to bitrot
> immediately... and break everyone else's patches and branches that touch
> the middle-end if/when it goes in (the "gimple" to "gimple *" change is
> the issue here).
>
> I'm thinking that I could attempt to put all of the handwritten stuff
> onto a (git) branch, or possibly put the .patch files though a renaming
> script to partially automated things first e.g. by updating the patches
> to use explicit pointers for *subclasses* as the patchkit introduces
> uses of subclasses.
>
> Then the big "gimple" to "gimple *" conversion could be done mostly
> separately, once the rest of the branch was ready to go in.  That would
> allow trunk to be easily mergable *into* the branch for all except the
> final autogenerated change.  We'd avoid introducing lots of "const_"
> typedefs, the only weird thing would be (until the final patch goes in)
> having "gimple" implicitly be a ptr, whereas the subclasses would
> require explicit ptr syntax.
>
> A merging strategy could be:
>  - keep the branch hand-written, with ChangeLog.gimple-classes files
>  - develop on branch, fixing more accessors, posting to gcc-patches
>  - merge from trunk to the branch periodically
>  - when ready, merge from branch to trunk in one commit
>  - apply the final autogenerated change of "gimple" to "gimple *" in one
> commit to trunk.
>
> That way everything is diffable, and the awkward autogenerated megapatch
> is isolated as one change.
>
> Thoughts?

Overall I'm fine with both conversions and any order they happen in
given both happen (I'm also fine to only do gimple -> gimple * for this
stage1).  I think timing-wise you should now have the burden to wait
for the end of stage1 (and thus all pending big merges).  I'm fine doing
this refactoring very early in stage3.  How you develop the patch is
left to you - I don't have strong opinions here.

Thanks,
Richard.

> Thanks for reading this far!
>
> Dave
>
>