This is the mail archive of the
gcc@gcc.gnu.org
mailing list for the GCC project.
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- From: David Malcolm <dmalcolm at redhat dot com>
- To: Richard Biener <richard dot guenther at gmail dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>, GCC Development <gcc at gcc dot gnu dot org>, Jeff Law <law at redhat dot com>
- Date: Mon, 17 Nov 2014 20:59:44 -0500
- Subject: Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Authentication-results: sourceware.org; auth=none
- References: <1415373690-26193-1-git-send-email-dmalcolm at redhat dot com> <1415373690-26193-5-git-send-email-dmalcolm at redhat dot com> <CAFiYyc2rg+T5d7rkTBPLeWKGnWMgv07j_N8KaFfr0NK1CjsfVQ at mail dot gmail dot com> <CAFiYyc2Y4uWt3Ec-D=c+LmKyBb1s0UPkP=8sSr1+5_nb7NruyA at mail dot gmail dot com> <20141108135646 dot GU5026 at tucnak dot redhat dot com> <1415658470 dot 2209 dot 20 dot camel at surprise> <20141111072614 dot GW5026 at tucnak dot redhat dot com> <CAFiYyc1jfx05qxvsCGqbdDL=ouH0hMp2A1ufandUbb-EfUAukg at mail dot gmail dot com> <1415842862 dot 2209 dot 101 dot camel at surprise> <CAFiYyc0H958TKBOb7F20hg8++YnOQYeMcQEZ2Ba+Q0F6hobeKw at mail dot gmail dot com> <1416049230 dot 21944 dot 35 dot camel at surprise> <CAFiYyc0T7W35JO0Om-oDL5RRr4ggSU0twvsGCn0PaGqkQAsBLA at mail dot gmail dot com>
On Mon, 2014-11-17 at 11:06 +0100, Richard Biener wrote:
> On Sat, Nov 15, 2014 at 12:00 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.
> >
> > I've created a branch "dmalcolm/gimple-classes-v2-option-3"
> > https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/gimple-classes-v2-option-3
> >
> > which takes the work reviewed by Jeff and the most trivial of the merger
> > followup work, throwing away the ~80 unloved followup patches on
> > dmalcolm/gimple-classes.
> >
> > I've merged from yesterday's trunk r217593 into that new branch,
> > resolving conflicts.
> >
> > I did this in two parts: the basic merger as
> > bd7fe714158f0c600caa05be7d744fd9139b8afb
> > resolving conflicts, with a followup patch to fixup new code from trunk
> > that used accessors that on the branch required a gimple subclass.
> >
> > Attached is that 2nd part of the merger.
> >
> > Successfully bootstrapped and regrtested on x86_64-unknown-linux-gnu;
> > same regrtest results as a control bootstrap of trunk's r217593.
> >
> > I appreciate Jakub and others have concerns about the overall approach.
> > I'm not sure which of option 2 (gimple-compat.h), option 3 (this one),
> > option 4 (just convert fields and non-accessor params), or defer to gcc
> > 6 is the best one, but I'm sleep-deprived and wanted to submit this
> > before the stage1 deadline.
> >
> > The other commits on this pruned branch that haven't been reviewed yet
> > are:
> >
> > [gimple-classes, committed 88/92] Preparatory work before subclass
> > renaming
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02820.html
>
> Ok.
>
> > [gimple-classes, committed 89/92] Eliminate subclass typedefs from
> > coretypes.h
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02838.html
>
> Ok.
>
> > [gimple-classes, committed 90/92] Automated renaming of gimple
> > subclasses
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02828.html
>
> Ok.
>
> > [gimple-classes, committed 91/92] Remove out-of-date references to
> > typedefs]
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02874.html
>
> Ok.
>
> > [gimple-classes, committed 92/92] Update gimple.texi class hierarchy
> > diagram
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02818.html
>
> Ok.
>
> > [gimple-classes] Merge trunk r216157-r216746 into branch
> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02982.html
>
> Ok.
Thanks. You said "Ok" to the various patches I pinged, but I don't
think you commented on the patch that was attached to the email.
Is that one OK? It's:
https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01935.html
in the archives.
I believe that's the only unreviewed patch on the branch
"dmalcolm/gimple-classes-v2-option-3":
https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/gimple-classes-v2-option-3
Assuming that's OK, I want to merge that branch to trunk in the next day
or so.
> > Also, presumably if this were merged, it would require a followup with
> > the gimple to gimple * fixup you wanted? (which we talked about doing as
> > an early stage3 thing IIRC [1]).
>
> Yeah, that would be nice (to remind people - this is about getting rid
> of const_gimple and thus avoids introducing tons of new const_
> for all the subclasses).
FWIW I got rid of all of those typedefs in the patches above (89/92 in
particular); the subclasses on the branch are already using explicit
ptrs, so it's more about consistency between base class ptrs and
subclass ptrs, to avoid:
gimple stmt;
gphi *phi;
gcall *call_stmt;
in favor of:
gimple *stmt; /* <-- note this one */
gphi *phi;
gcall *call_stmt;
I'd already done that for the subclasses on the branch, and the diff
between the branch and trunk is relatively sane.
By contrast, doing the gimple -> gimple * fixup will generate a huge
diff, so that's something to do after merging the branch.
I'll post the patch here later this week (or a link to it, it's likely
to be too big even compressed for the ML).
Dave
> Thanks,
> Richard.
>
> > Thanks
> > Dave
> > [1] e.g. https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01536.html
> >
> >
> >> > 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.
> >>
> >> >> 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
> >> >
> >
- References:
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
- Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign