Re: [PATCH] Add gimple-compat.h (was Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign)

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

On Fri, Nov 14, 2014 at 4:27 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Thu, 2014-11-13 at 11:45 +0100, Richard Biener wrote:
>> On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm <dmalcolm@redhat.com> wrote:
>> > On Tue, 2014-11-11 at 11:43 +0100, Richard Biener wrote:
>> >> On Tue, Nov 11, 2014 at 8:26 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>> >> > On Mon, Nov 10, 2014 at 05:27:50PM -0500, David Malcolm wrote:
>> >> >> On Sat, 2014-11-08 at 14:56 +0100, Jakub Jelinek wrote:
>> >> >> > On Sat, Nov 08, 2014 at 01:07:28PM +0100, Richard Biener wrote:
>> >> >> > > To be constructive here - the above case is from within a
>> >> >> > > GIMPLE_ASSIGN case label
>> >> >> > > and thus I'd have expected
>> >> >> > >
>> >> >> > >     case GIMPLE_ASSIGN:
>> >> >> > >       {
>> >> >> > >         gassign *a1 = as_a <gassign *> (s1);
>> >> >> > >         gassign *a2 = as_a <gassign *> (s2);
>> >> >> > >       lhs1 = gimple_assign_lhs (a1);
>> >> >> > >       lhs2 = gimple_assign_lhs (a2);
>> >> >> > >       if (TREE_CODE (lhs1) != SSA_NAME
>> >> >> > >           && TREE_CODE (lhs2) != SSA_NAME)
>> >> >> > >         return (operand_equal_p (lhs1, lhs2, 0)
>> >> >> > >                 && gimple_operand_equal_value_p (gimple_assign_rhs1 (a1),
>> >> >> > >                                                  gimple_assign_rhs1 (a2)));
>> >> >> > >       else if (TREE_CODE (lhs1) == SSA_NAME
>> >> >> > >                && TREE_CODE (lhs2) == SSA_NAME)
>> >> >> > >         return vn_valueize (lhs1) == vn_valueize (lhs2);
>> >> >> > >       return false;
>> >> >> > >       }
>> >> >> > >
>> >> >> > > instead.  That's the kind of changes I have expected and have approved of.
>> >> >> >
>> >> >> > But even that looks like just adding extra work for all developers, with no
>> >> >> > gain.  You only have to add extra code and extra temporaries, in switches
>> >> >> > typically also have to add {} because of the temporaries and thus extra
>> >> >> > indentation level, and it doesn't simplify anything in the code.
>> >> >>
>> >> >> The branch attempts to use the C++ typesystem to capture information
>> >> >> about the kinds of gimple statement we expect, both:
>> >> >>   (A) so that the compiler can detect type errors, and
>> >> >>   (B) as a comprehension aid to the human reader of the code
>> >> >>
>> >> >> The ideal here is when function params and struct field can be
>> >> >> strengthened from "gimple" to a subclass ptr.  This captures the
>> >> >> knowledge that every use of a function or within a struct has a given
>> >> >> gimple code.
>> >> >
>> >> > I just don't like all the as_a/is_a stuff enforced everywhere,
>> >> > it means more typing, more temporaries, more indentation.
>> >> > So, as I view it, instead of the checks being done cheaply (yes, I think
>> >> > the gimple checking as we have right now is very cheap) under the
>> >> > hood by the accessors (gimple_assign_{lhs,rhs1} etc.), those changes
>> >> > put the burden on the developers, who has to check that manually through
>> >> > the as_a/is_a stuff everywhere, more typing and uglier syntax.
>> >> > I just don't see that as a step forward, instead a huge step backwards.
>> >> > But perhaps I'm alone with this.
>> >> > Can you e.g. compare the size of - lines in your patchset combined, and
>> >> > size of + lines in your patchset?  As in, if your changes lead to less
>> >> > typing or more.
>> >>
>> >> I see two ways out here.  One is to add overloads to all the functions
>> >> taking the special types like
>> >>
>> >> tree
>> >> gimple_assign_rhs1 (gimple *);
>> >>
>> >> or simply add
>> >>
>> >> gassign *operator ()(gimple *g) { return as_a <gassign *> (g); }
>> >>
>> >> into a gimple-compat.h header which you include in places that
>> >> are not converted "nicely".
>> >
>> > Thanks for the suggestions.
>> >
>> > Am I missing something, or is the gimple-compat.h idea above not valid C
>> > ++?
>> >
>> > Note that "gimple" is still a typedef to
>> >   gimple_statement_base *
>> > (as noted before, the gimple -> gimple * change would break everyone
>> > else's patches, so we talked about that as a followup patch for early
>> > stage3).
>> >
>> > Given that, if I try to create an "operator ()" outside of a class, I
>> > get this error:
>> >
>> > âgassign* operator()(gimple)â must be a nonstatic member function
>> >
>> > which is emitted from cp/decl.c's grok_op_properties:
>> >       /* An operator function must either be a non-static member function
>> >          or have at least one parameter of a class, a reference to a class,
>> >          an enumeration, or a reference to an enumeration.  13.4.0.6 */
>> >
>> > I tried making it a member function of gimple_statement_base, but that
>> > doesn't work either: we want a conversion
>> >   from a gimple_statement_base * to a gassign *, not
>> >   from a gimple_statement_base   to a gassign *.
>> >
>> > Is there some syntactic trick here that I'm missing?  Sorry if I'm being
>> > dumb (I can imagine there's a way of doing it by making "gimple" become
>> > some kind of wrapped ptr class, but that way lies madness, surely).
>>
>> Hmm.
>>
>> struct assign;
>> struct base {
>>   operator assign *() const { return (assign *)this; }
>> };
>> struct assign : base {
>> };
>>
>> void foo (assign *);
>> void bar (base *b)
>> {
>>   foo (b);
>> }
>>
>> doesn't work, but
>>
>> void bar (base &b)
>> {
>>   foo (b);
>> }
>>
>> does.  Indeed C++ doesn't seem to provide what is necessary
>> for the compat trick :(
>>
>> So the gimple-compat.h header would need to provide
>> additional overloads for the affected functions like
>>
>> inline tree
>> gimple_assign_rhs1 (gimple *g)
>> {
>>   return gimple_assign_rhs1 (as_a <gassign *> (g));
>> }
>>
>> that would work for me as well.
>>
>> >> Both avoid manually making the compiler happy (which the
>> >> explicit as_a<> stuff is!  It doesn't add any "checking" - it's
>> >> just placing the as_a<> at the callers and thus make the
>> >> runtine ICE fire there).
>> >>
>> >> As much as I don't like "global" conversion operators I don't
>> >> like adding overloads to all of the accessor functions even more.
>> >
>> > (nods)
>> >
>> > Some other options:
>> >
>> > Option 3: only convert the "easy" accessors: the ones I already did in
>> > the /89 patch kit, as reviewed by Jeff, and rebased by me recently,
>> > which is this 92-patch kit:
>> >   "[gimple-classes, committed 00/92] Initial slew of commits":
>> >      https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html
>> > Doing so converts about half of the gimple_foo_ accessors to taking a
>> > gfoo *, giving a mixture of GIMPLE_CHECK vs subclass use.   I believe
>> > the quality of those patches was higher than the later ones on the
>> > branch: I was doing the places that didn't require the invasive/verbose
>> > changes seen in the later patches.  Shelve the remaining ~80
>> > increasingly ugly patches, starting a new branch to contain just the
>> > good ones.
>> >
>> >
>> > Option 4: don't convert any accessors, but instead focus on fields of
>> > structs (e.g. "call_stmt" within a cgraph_edge), and on params of other
>> > functions (e.g. phi-manipulation code).  That way we'd avoid the
>> > inconsistency of some accessors using GIMPLE_CHECK and some using
>> > subclasses - all would continue to consistently use GIMPLE_CHECK, but
>> > there would be some extra type-checking and self-documentation of the
>> > expected statement kinds in the code.
>> >
>> >
>> >
>> > FWIW, option 3 is my preferred approach (I've already done the bulk of
>> > the work and it's already been reviewed; it would need an update merger
>> > from trunk, and there's the big gimple to gimple * fixup you wanted).
>>
>> Works for me as well.  The compat solution looks somewhat appealing
>> as we can then incrementally fix up things rather than requiring to
>> mass-convert everything.
>>
>> Thanks,
>> Richard.
>
> I still prefer option 3 above (i.e. go with the patches Jeff reviewed,
> with the fixups discussed earlier, with a merger from trunk), but I had
> a go at implementing your gimple-compat.h idea.
>
> Attached is a patch against e74b2ecd78d21fc7bcc2f50dacc92819e0e69e6b on
> the gimple-classes branch (the last commit before I went overboard with
> the as_a changes), which converts the remaining gimple_assign_ accessors
> to take a gassign *, adding a gimple-compat.h to provide overloaded
> accessors taking just a gimple for those source files that needed them.
> It also strengthens some places I spotted where converting params,
> locals or fields from gimple to gassign * was simple and obvious.

Thanks.

> Successfully bootstrapped&regrtested on x86_64-unknown-linux-gnu,
> comparing the branch+patch against master r216746 (2014-10-27).
>
> This only covers gimple_assign_; it would still be a hybrid of is-a and
> GIMPLE_CHECK unless I extended the patch to cover all the other
> accessors.
>
> Do you want me to continue converting things for gimple-compat.h to see
> what a set of patches for that approach looks like?

I think it would be reasonable to try pushing things a bit further - esp.
providing the strongly typed interfaces by default to make new code
"forcibly" use that.

> If so, would it be better to split things out and have a
> gimple-compat-assign.h, gimple-compat-call.h etc rather than one big
> gimple-compat.h?

No, I think one gimple-compat.h is enough.

> That said, I still prefer option 3.

I think it's fine to push option 3 now (thus commit that).  I think
doing the gimple-compat.h in addition would be nice.

Thanks,
Richard.

>
>> >> Whether you enable them generally or just for selected files
>> >> via a gimple-compat.h will be up to you (but I'd rather get
>> >> rid of them at some point).
>> >>
>> >> Note this allows seamless transform of "random" functions
>> >> taking a gimple now but really only expecting a single kind.
>> >>
>> >> Note that we don't absolutely have to rush this all in for GCC 5.
>> >> Being the very first for GCC 6 stage1 is another possibility.
>> >> We just should get it right.
>> >
>> > Thanks
>> > Dave
>> >
>