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 11/13/2014 05:45 AM, 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.


Won't that defeat the desire for checking tho? If you dont do a dyn_cast<> in gimple_assign_rhs1 (gimple *g) anyone can call it and upcast any kind of gimple into a gassign without checking that it is really a gassign... Actually, a gcc_assert here may be sufficient... and better.

It'd be pretty easy to miss someone calling it when its not a gassign.

Andrew



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