Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Tue, Nov 18, 2014 at 10:04 PM, David Malcolm dmalc...@redhat.com wrote:
On Tue, 2014-11-18 at 10:53 +0100, Richard Biener wrote:
On Tue, Nov 18, 2014 at 2:59 AM, David Malcolm dmalc...@redhat.com wrote:
On Mon, 2014-11-17 at 11:06 +0100, Richard Biener wrote:
On Sat, Nov 15, 2014 at 12:00 PM, David Malcolm dmalc...@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 dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Tue, Nov 18, 2014 at 2:59 AM, David Malcolm dmalc...@redhat.com wrote:
On Mon, 2014-11-17 at 11:06 +0100, Richard Biener wrote:
On Sat, Nov 15, 2014 at 12:00 PM, David Malcolm dmalc...@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 dmalc...@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 ja...@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; }
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Tue, 2014-11-18 at 10:53 +0100, Richard Biener wrote:
On Tue, Nov 18, 2014 at 2:59 AM, David Malcolm dmalc...@redhat.com wrote:
On Mon, 2014-11-17 at 11:06 +0100, Richard Biener wrote:
On Sat, Nov 15, 2014 at 12:00 PM, David Malcolm dmalc...@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 dmalc...@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 ja...@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
Re: [PATCH] Add gimple-compat.h (was Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign)
On Fri, Nov 14, 2014 at 4:27 PM, David Malcolm dmalc...@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 dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Sat, Nov 15, 2014 at 12:00 PM, David Malcolm dmalc...@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 dmalc...@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 ja...@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
Re: [PATCH] Add gimple-compat.h (was Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign)
On 11/14/14 08:27, David Malcolm wrote:
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.
So, I'm chiming in a bit late, but just want to touch on a few things.
First, as I've stated before, I see as_a/is_a as generally a wart for
things we still need to cleanup and redesign. I do not want to see them
sprinkled throughout GCC. If we find ourselves adding a bunch of these,
then we've got some redesign/rethinking that needs to be done.
Yes, I know some will be necessary and some are more like markers for
the limits of where we are with the gimple class work, particularly
since we're trying to stage in this work rather than convert everything
at once (which I believe, realistically, is impossible).
I see two ways out here. One is to add overloads to all the functions
taking the special types like
IIRC Andrew was doing similar things as a temporary measure in the
gimple/tree type work as well. Basically the overloads were to allow
the two schemes to co-exist while conversions were done with the express
intent that the overloads were to disappear when conversion is complete.
I'd be comfortable with a similar mechanism for this work as well.
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.
And this would be my preferred option for where we are today. That ~89
kit was a step in the right direction. I think going beyond that for
the close of stage1 was ambitious :-)
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.
Exactly. And it's real easy to see what's depending on those overloads
as one can simply remove them and try to build. In many ways, I'd
prefer the temporary overload solution for the next round of work in
this space so that conversions can occur piecemeal instead of in large
series patchsets.
I've got no objection if we have the compat hack in now. I haven't
reviewed that work, just no philosophical objections :-) I would be
opposed to pushing the gimple class work further than that prior to the
next stage1 opening.
jeff
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On 11/17/14 03:06, Richard Biener wrote: 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). Right. Mirrors what we've done for INSNs in RTL as well. jeff
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Mon, 2014-11-17 at 11:06 +0100, Richard Biener wrote:
On Sat, Nov 15, 2014 at 12:00 PM, David Malcolm dmalc...@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 dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Thu, 2014-11-13 at 11:45 +0100, Richard Biener wrote:
On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm dmalc...@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 ja...@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
[PATCH] Add gimple-compat.h (was Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign)
On Thu, 2014-11-13 at 11:45 +0100, Richard Biener wrote:
On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On 13 November 2014 10:45, Richard Biener wrote:
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 :(
Right, base* is a built-in type, you can't call a member function on it.
There is no implicit conversion between unrelated pointer types, and
no implicit conversion from base* to base that would be necessary to
call the conversion operator of base.
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 dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Thu, Nov 13, 2014 at 3:24 PM, Andrew MacLeod amacl...@redhat.com wrote:
On 11/13/2014 05:45 AM, Richard Biener wrote:
On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm dmalc...@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 ja...@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.
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On 11/13/2014 09:34 AM, Richard Biener wrote:
On Thu, Nov 13, 2014 at 3:24 PM, Andrew MacLeod amacl...@redhat.com wrote:
On 11/13/2014 05:45 AM, Richard Biener wrote:
On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm dmalc...@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 ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Tue, 2014-11-11 at 11:43 +0100, Richard Biener wrote:
On Tue, Nov 11, 2014 at 8:26 AM, Jakub Jelinek ja...@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).
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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
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.
IMO that's the sort of things some of us were afraid of when the C++ switch
was being discussed and IIRC we were told this would not happen...
--
Eric Botcazou
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Tue, Nov 11, 2014 at 8:26 AM, Jakub Jelinek ja...@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
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On 11/11/2014 09:30 AM, Eric Botcazou wrote:
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.
IMO that's the sort of things some of us were afraid of when the C++ switch
was being discussed and IIRC we were told this would not happen...
I'm with both of you on this.
Bernd
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
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.
Examples of this for the initial patchkit were:
* the call_stmt field of a cgraph_edge becoming a gcall *,
rather than a plain gimple.
* various variables in tree-into-ssa.c change from just
vecgimple to being vecgphi *, capturing the phi-ness of
the contents as a compile-time check
* tree-inline.h's struct copy_body_data, the field debug_stmts
can be concretized from a vecgimple to a vecgdebug *.
A more recent example, from:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5bd16d92b9e928b5a5a7aebd571d92f72dd517f8
The fields arr_ref_first and arr_ref_last of
tree-switch-conversion.c's struct switch_conv_info can be strengthened
from gimple to gassign *: they are always GIMPLE_ASSIGN.
I applied cleanups to do my initial patchkit, which Jeff approved (with
some provisos), and which became the first 92 commits on the branch:
[gimple-classes, committed 00/92] Initial slew of commits:
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html
followed by a merger from trunk into the branch:
[gimple-classes] Merge trunk r216157-r216746 into branch:
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02982.html
With those commits, I was able to convert 180 accessors to taking a
concrete subclass, with 158 left taking a gimple or
const_gimple i.e. about half of them.
(My script to analyze this is gimple_typesafety.py
within https://github.com/davidmalcolm/gcc-refactoring-scripts)
I got it into my head that it was desirable to convert *all*
gimple accessors to this form, and to eliminate the GIMPLE_CHECK
macros (given that gcc development community seems to dislike
partial transitions).
I've been attempting this full conversion - convert all of the
gimple_ accessors, to require an appropriate gimple subclass
ptr, in particular focusing on the gimple_assign_ ones, but it's a *lot*
of extra work, and much more invasive than the patches
that Jeff conditionally approved.
I now suspect that it's going too far - in the initial patchkit I was
doing the clean, obvious ones, but now I'm left with the awkward ones
that would require me to uglify the code to fix.
If it's OK to only convert some of them, then I'd rather just do that.
The type-strengthening is rarely as neat as being able to simply convert
a param or field type. Some examples:
Functions passed a gsi
==
Sometimes functions are passed a gsi, where it can be known that the gsi
currently references a stmt of known kind (although that isn't
necessarily obvious from reading the body of the function):
Example from tree-ssa-strlen.c:
handle_char_store (gimple_stmt_iterator *gsi)
{
int idx = -1;
strinfo si = NULL;
- gimple stmt = gsi_stmt (*gsi);
+ gassign *stmt = as_a gassign * (gsi_stmt (*gsi));
tree ssaname = NULL_TREE, lhs = gimple_assign_lhs (stmt);
if (TREE_CODE (lhs) == MEM_REF
from
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=78aae552f15ad5f8f5290fb825f9ae33f4a7cad9
Only acting on one kind of gimple
=
Some functions accept any kind of gimple, but only act on e.g. a
GIMPLE_ASSIGN, immediately returning if they got a different kind.
So I make this kind of change, where:
void
foo (gimple stmt, other params)
{
if (!is_gimple_assign (stmt))
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Mon, Nov 10, 2014 at 2:27 PM, David Malcolm dmalc...@redhat.com 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.
Examples of this for the initial patchkit were:
* the call_stmt field of a cgraph_edge becoming a gcall *,
rather than a plain gimple.
* various variables in tree-into-ssa.c change from just
vecgimple to being vecgphi *, capturing the phi-ness of
the contents as a compile-time check
* tree-inline.h's struct copy_body_data, the field debug_stmts
can be concretized from a vecgimple to a vecgdebug *.
A more recent example, from:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=5bd16d92b9e928b5a5a7aebd571d92f72dd517f8
The fields arr_ref_first and arr_ref_last of
tree-switch-conversion.c's struct switch_conv_info can be strengthened
from gimple to gassign *: they are always GIMPLE_ASSIGN.
I applied cleanups to do my initial patchkit, which Jeff approved (with
some provisos), and which became the first 92 commits on the branch:
[gimple-classes, committed 00/92] Initial slew of commits:
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html
followed by a merger from trunk into the branch:
[gimple-classes] Merge trunk r216157-r216746 into branch:
https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02982.html
With those commits, I was able to convert 180 accessors to taking a
concrete subclass, with 158 left taking a gimple or
const_gimple i.e. about half of them.
(My script to analyze this is gimple_typesafety.py
within https://github.com/davidmalcolm/gcc-refactoring-scripts)
I got it into my head that it was desirable to convert *all*
gimple accessors to this form, and to eliminate the GIMPLE_CHECK
macros (given that gcc development community seems to dislike
partial transitions).
I've been attempting this full conversion - convert all of the
gimple_ accessors, to require an appropriate gimple subclass
ptr, in particular focusing on the gimple_assign_ ones, but it's a *lot*
of extra work, and much more invasive than the patches
that Jeff conditionally approved.
I now suspect that it's going too far - in the initial patchkit I was
doing the clean, obvious ones, but now I'm left with the awkward ones
that would require me to uglify the code to fix.
If it's OK to only convert some of them, then I'd rather just do that.
The type-strengthening is rarely as neat as being able to simply convert
a param or field type. Some examples:
Functions passed a gsi
==
Sometimes functions are passed a gsi, where it can be known that the gsi
currently references a stmt of known kind (although that isn't
necessarily obvious from reading the body of the function):
Example from tree-ssa-strlen.c:
handle_char_store (gimple_stmt_iterator *gsi)
{
int idx = -1;
strinfo si = NULL;
- gimple stmt = gsi_stmt (*gsi);
+ gassign *stmt = as_a gassign * (gsi_stmt (*gsi));
Can we have something like:
gsi_assign (*gsi)
instead so there is less typing when we want an gassign rather than a
gimple stmt. This should allow for easier converting also and puts
most of the as_a in one place.
Thanks,
Andrew Pinski
tree ssaname = NULL_TREE, lhs = gimple_assign_lhs (stmt);
if (TREE_CODE (lhs) == MEM_REF
from
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
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.
Jakub
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Fri, Nov 07, 2014 at 10:01:45PM +0100, Richard Biener wrote: Just a comment as these patches flow by - I think this is a huge step backwards from enforcing s1/s2 being a gimple_assign inside gimple_assign_rhs1 to this as_a gassign * boilerplate at _each_ callsite! FWIW, I feel the same way. More to type, worse readability, a lot more of line-wrapping. Sorry, Marek
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Fri, Nov 7, 2014 at 10:01 PM, Richard Biener
richard.guent...@gmail.com wrote:
On Fri, Nov 7, 2014 at 4:21 PM, David Malcolm dmalc...@redhat.com wrote:
gcc/ChangeLog.gimple-classes:
* tree-ssa-tail-merge.c (same_succ_hash): Add checked cast.
(gimple_equal_p): Add checked casts.
---
gcc/ChangeLog.gimple-classes | 5 +
gcc/tree-ssa-tail-merge.c| 8 +---
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/gcc/ChangeLog.gimple-classes b/gcc/ChangeLog.gimple-classes
index f43df63..0bd0421 100644
--- a/gcc/ChangeLog.gimple-classes
+++ b/gcc/ChangeLog.gimple-classes
@@ -1,5 +1,10 @@
2014-11-06 David Malcolm dmalc...@redhat.com
+ * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast.
+ (gimple_equal_p): Add checked casts.
+
+2014-11-06 David Malcolm dmalc...@redhat.com
+
* tree-ssa-structalias.c (find_func_aliases): Replace
is_gimple_assign with a dyn_cast, introducing local gassign *
t_assign, using it in place of t for typesafety.
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 5678657..b822214 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -484,7 +484,7 @@ same_succ_hash (const_same_succ e)
hstate.add_int (gimple_code (stmt));
if (is_gimple_assign (stmt))
- hstate.add_int (gimple_assign_rhs_code (stmt));
+ hstate.add_int (gimple_assign_rhs_code (as_a gassign * (stmt)));
if (!is_gimple_call (stmt))
continue;
if (gimple_call_internal_p (stmt))
@@ -1172,8 +1172,10 @@ gimple_equal_p (same_succ same_succ, gimple s1,
gimple s2)
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 (s1),
-gimple_assign_rhs1 (s2)));
+gimple_operand_equal_value_p (gimple_assign_rhs1 (
+ as_a gassign * (s1)),
+gimple_assign_rhs1 (
+ as_a gassign * (s2;
Just a comment as these patches flow by - I think this is a huge step
backwards from enforcing s1/s2 being a gimple_assign inside
gimple_assign_rhs1 to this as_a gassign * boilerplate at _each_ callsite!
Which means this step of the refactoring is totally broken and probably
requires much more manual work to avoid this kind of uglyness.
I definitely won't approve of this kind of changes.
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.
Thanks,
Richard.
Thanks,
Richard.
else if (TREE_CODE (lhs1) == SSA_NAME
TREE_CODE (lhs2) == SSA_NAME)
return vn_valueize (lhs1) == vn_valueize (lhs2);
--
1.7.11.7
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
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.
Jakub
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Sat, 2014-11-08 at 13:07 +0100, Richard Biener wrote:
On Fri, Nov 7, 2014 at 10:01 PM, Richard Biener
richard.guent...@gmail.com wrote:
On Fri, Nov 7, 2014 at 4:21 PM, David Malcolm dmalc...@redhat.com wrote:
gcc/ChangeLog.gimple-classes:
* tree-ssa-tail-merge.c (same_succ_hash): Add checked cast.
(gimple_equal_p): Add checked casts.
---
gcc/ChangeLog.gimple-classes | 5 +
gcc/tree-ssa-tail-merge.c| 8 +---
2 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/gcc/ChangeLog.gimple-classes b/gcc/ChangeLog.gimple-classes
index f43df63..0bd0421 100644
--- a/gcc/ChangeLog.gimple-classes
+++ b/gcc/ChangeLog.gimple-classes
@@ -1,5 +1,10 @@
2014-11-06 David Malcolm dmalc...@redhat.com
+ * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast.
+ (gimple_equal_p): Add checked casts.
+
+2014-11-06 David Malcolm dmalc...@redhat.com
+
* tree-ssa-structalias.c (find_func_aliases): Replace
is_gimple_assign with a dyn_cast, introducing local gassign *
t_assign, using it in place of t for typesafety.
diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c
index 5678657..b822214 100644
--- a/gcc/tree-ssa-tail-merge.c
+++ b/gcc/tree-ssa-tail-merge.c
@@ -484,7 +484,7 @@ same_succ_hash (const_same_succ e)
hstate.add_int (gimple_code (stmt));
if (is_gimple_assign (stmt))
- hstate.add_int (gimple_assign_rhs_code (stmt));
+ hstate.add_int (gimple_assign_rhs_code (as_a gassign * (stmt)));
if (!is_gimple_call (stmt))
continue;
if (gimple_call_internal_p (stmt))
@@ -1172,8 +1172,10 @@ gimple_equal_p (same_succ same_succ, gimple s1,
gimple s2)
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 (s1),
-gimple_assign_rhs1 (s2)));
+gimple_operand_equal_value_p (gimple_assign_rhs1 (
+ as_a gassign * (s1)),
+gimple_assign_rhs1 (
+ as_a gassign *
(s2;
Just a comment as these patches flow by - I think this is a huge step
backwards from enforcing s1/s2 being a gimple_assign inside
gimple_assign_rhs1 to this as_a gassign * boilerplate at _each_ callsite!
Which means this step of the refactoring is totally broken and probably
requires much more manual work to avoid this kind of uglyness.
I definitely won't approve of this kind of changes.
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.
I do make the above kind of change in some places within the
gimple-classes branch.
I think I didn't do it in this case because the body of the case
GIMPLE_ASSIGN doesn't yet have braces, so adding locals requires adding
them and re-indenting the case body. I didn't spot the opportunity to
speed up the code as you do above by converting the two gimple_get_lhs
to gimple_assign_lhs. Without that, I guess I decided to simply add the
two as_a directly in-place to avoid the reindent. With your speedup
it's clearly better to reindent the code.
(Got to go now, sorry; I hope to write a better reply on Monday)
Thanks
Dave
[gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
gcc/ChangeLog.gimple-classes: * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast. (gimple_equal_p): Add checked casts. --- gcc/ChangeLog.gimple-classes | 5 + gcc/tree-ssa-tail-merge.c| 8 +--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/gcc/ChangeLog.gimple-classes b/gcc/ChangeLog.gimple-classes index f43df63..0bd0421 100644 --- a/gcc/ChangeLog.gimple-classes +++ b/gcc/ChangeLog.gimple-classes @@ -1,5 +1,10 @@ 2014-11-06 David Malcolm dmalc...@redhat.com + * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast. + (gimple_equal_p): Add checked casts. + +2014-11-06 David Malcolm dmalc...@redhat.com + * tree-ssa-structalias.c (find_func_aliases): Replace is_gimple_assign with a dyn_cast, introducing local gassign * t_assign, using it in place of t for typesafety. diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c index 5678657..b822214 100644 --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -484,7 +484,7 @@ same_succ_hash (const_same_succ e) hstate.add_int (gimple_code (stmt)); if (is_gimple_assign (stmt)) - hstate.add_int (gimple_assign_rhs_code (stmt)); + hstate.add_int (gimple_assign_rhs_code (as_a gassign * (stmt))); if (!is_gimple_call (stmt)) continue; if (gimple_call_internal_p (stmt)) @@ -1172,8 +1172,10 @@ gimple_equal_p (same_succ same_succ, gimple s1, gimple s2) 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 (s1), -gimple_assign_rhs1 (s2))); +gimple_operand_equal_value_p (gimple_assign_rhs1 ( + as_a gassign * (s1)), +gimple_assign_rhs1 ( + as_a gassign * (s2; else if (TREE_CODE (lhs1) == SSA_NAME TREE_CODE (lhs2) == SSA_NAME) return vn_valueize (lhs1) == vn_valueize (lhs2); -- 1.7.11.7
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Fri, Nov 7, 2014 at 4:21 PM, David Malcolm dmalc...@redhat.com wrote: gcc/ChangeLog.gimple-classes: * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast. (gimple_equal_p): Add checked casts. --- gcc/ChangeLog.gimple-classes | 5 + gcc/tree-ssa-tail-merge.c| 8 +--- 2 files changed, 10 insertions(+), 3 deletions(-) diff --git a/gcc/ChangeLog.gimple-classes b/gcc/ChangeLog.gimple-classes index f43df63..0bd0421 100644 --- a/gcc/ChangeLog.gimple-classes +++ b/gcc/ChangeLog.gimple-classes @@ -1,5 +1,10 @@ 2014-11-06 David Malcolm dmalc...@redhat.com + * tree-ssa-tail-merge.c (same_succ_hash): Add checked cast. + (gimple_equal_p): Add checked casts. + +2014-11-06 David Malcolm dmalc...@redhat.com + * tree-ssa-structalias.c (find_func_aliases): Replace is_gimple_assign with a dyn_cast, introducing local gassign * t_assign, using it in place of t for typesafety. diff --git a/gcc/tree-ssa-tail-merge.c b/gcc/tree-ssa-tail-merge.c index 5678657..b822214 100644 --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -484,7 +484,7 @@ same_succ_hash (const_same_succ e) hstate.add_int (gimple_code (stmt)); if (is_gimple_assign (stmt)) - hstate.add_int (gimple_assign_rhs_code (stmt)); + hstate.add_int (gimple_assign_rhs_code (as_a gassign * (stmt))); if (!is_gimple_call (stmt)) continue; if (gimple_call_internal_p (stmt)) @@ -1172,8 +1172,10 @@ gimple_equal_p (same_succ same_succ, gimple s1, gimple s2) 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 (s1), -gimple_assign_rhs1 (s2))); +gimple_operand_equal_value_p (gimple_assign_rhs1 ( + as_a gassign * (s1)), +gimple_assign_rhs1 ( + as_a gassign * (s2; Just a comment as these patches flow by - I think this is a huge step backwards from enforcing s1/s2 being a gimple_assign inside gimple_assign_rhs1 to this as_a gassign * boilerplate at _each_ callsite! Which means this step of the refactoring is totally broken and probably requires much more manual work to avoid this kind of uglyness. I definitely won't approve of this kind of changes. Thanks, Richard. else if (TREE_CODE (lhs1) == SSA_NAME TREE_CODE (lhs2) == SSA_NAME) return vn_valueize (lhs1) == vn_valueize (lhs2); -- 1.7.11.7
Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign
On Fri, Nov 07, 2014 at 10:01:45PM +0100, Richard Biener wrote: --- a/gcc/tree-ssa-tail-merge.c +++ b/gcc/tree-ssa-tail-merge.c @@ -484,7 +484,7 @@ same_succ_hash (const_same_succ e) hstate.add_int (gimple_code (stmt)); if (is_gimple_assign (stmt)) - hstate.add_int (gimple_assign_rhs_code (stmt)); + hstate.add_int (gimple_assign_rhs_code (as_a gassign * (stmt))); if (!is_gimple_call (stmt)) continue; if (gimple_call_internal_p (stmt)) @@ -1172,8 +1172,10 @@ gimple_equal_p (same_succ same_succ, gimple s1, gimple s2) 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 (s1), -gimple_assign_rhs1 (s2))); +gimple_operand_equal_value_p (gimple_assign_rhs1 ( + as_a gassign * (s1)), +gimple_assign_rhs1 ( + as_a gassign * (s2; Just a comment as these patches flow by - I think this is a huge step backwards from enforcing s1/s2 being a gimple_assign inside gimple_assign_rhs1 to this as_a gassign * boilerplate at _each_ callsite! Which means this step of the refactoring is totally broken and probably requires much more manual work to avoid this kind of uglyness. I definitely won't approve of this kind of changes. I have to agree with this, this is too ugly to live with. I must say I don't find anything wrong with what we have right now, unlike RTL checking, the gimple checking is inexpensive, and much better to do it that way then enforce all all developers to write it this way. Otherwise we'll end up with code as ugly as in LLVM :(. Jakub
