This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign


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

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.

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,
Richard.


>         Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]