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

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

[David Malcolm]

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?)

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?

Thanks for reading this far!

Dave

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

[Richard Biener]

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
>
>

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

[Jeff Law]

On 10/16/14 05:27, Richard Biener wrote:
>>
>> 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.
If they're fairly mechanical, then I think we're OK.

>>
>> 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
OK.

>> 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.
No strong opinions here other than I think this stuff should go forward 
during this stage1/early stage3.  David delayed the work until after 
4.9.1 was released and is doing a massive renaming -- we shouldn't 
penalize him for meeting our requests.  So as long as we're in agreement 
this stuff is suitable now or early in stage3, then I'm comfortable with 
either approach.

Jeff

[gimple-classes] New branch (was Re: [RFC] Compile-time gimple-checking (again))

[David Malcolm]

On Thu, 2014-10-16 at 12:36 -0600, Jeff Law wrote:
> On 10/16/14 05:27, Richard Biener wrote:
> >>
> >> 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.
> If they're fairly mechanical, then I think we're OK.
> 
> >>
> >> 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
> OK.
> 
> >> 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.
> No strong opinions here other than I think this stuff should go forward 
> during this stage1/early stage3.  David delayed the work until after 
> 4.9.1 was released and is doing a massive renaming -- we shouldn't 
> penalize him for meeting our requests.  So as long as we're in agreement 
> this stuff is suitable now or early in stage3, then I'm comfortable with 
> either approach.

Thanks.

I've created a new branch within git: "dmalcolm/gimple-classes" to track
this work.

It can be seen at:
https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/dmalcolm/gimple-classes
(branching from r216157 aka fc222f445c6108418196a1b48703d350f3c3d45a)

I've pushed 92 patches there so far, which I'm about to post to this
list, with the prefix "[gimple-classes]".

I'm accumulating the ChangeLogs into a gcc/ChangeLog.gimple-classes
file, as well as in the commit messages.

Dave