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: Richard Biener <richard dot guenther at gmail dot com>
- To: David Malcolm <dmalcolm at redhat 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: Thu, 13 Nov 2014 11:45:17 +0100
- 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>
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.
>> 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
>