This is the mail archive of the
gcc-patches@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: Jakub Jelinek <jakub at redhat dot com>
- Cc: Richard Biener <richard dot guenther at gmail 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: Wed, 12 Nov 2014 21:12:53 -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>
On Tue, 2014-11-11 at 08:26 +0100, Jakub Jelinek 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.
I agree with you w.r.t. my later patches. I like my initial set of 89
patches, but I overdid things with the attempt to convert all of the
gimple_assign_ accessors.
> 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 don't know if this data matches the proposed interpretation
(autoindentation in emacs is wonderful), but here goes:
Diff of my current working copy vs last merge point, ignoring ChangeLog
additions, obtaining lines starting "-", piping into wc:
$ git diff fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 $(git diff
fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 | diffstat -lp1 | grep -v
ChangeLog) | grep -e "^-" | wc
6169 31032 272916
Likewise for lines starting with "+":
$ git diff fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 $(git diff
fddbd0194b01f44c5b5f16379fd5405dcf6d71c0 | diffstat -lp1 | grep -v
ChangeLog) | grep -e "^+" | wc
7295 36566 309380
so the + lines have 13% more bytes than the - lines
Dave