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<> stuff is! It doesn't add any "checking" - it's >> just placing the as_a<> at the callers and thus make the >> runtine ICE fire there). >> >> As much as I don't like "global" conversion operators I don't >> like adding overloads to all of the accessor functions even more. > > (nods) > > Some other options: > > Option 3: only convert the "easy" accessors: the ones I already did in > the /89 patch kit, as reviewed by Jeff, and rebased by me recently, > which is this 92-patch kit: > "[gimple-classes, committed 00/92] Initial slew of commits": > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02791.html > Doing so converts about half of the gimple_foo_ accessors to taking a > gfoo *, giving a mixture of GIMPLE_CHECK vs subclass use. I believe > the quality of those patches was higher than the later ones on the > branch: I was doing the places that didn't require the invasive/verbose > changes seen in the later patches. Shelve the remaining ~80 > increasingly ugly patches, starting a new branch to contain just the > good ones. > > > Option 4: don't convert any accessors, but instead focus on fields of > structs (e.g. "call_stmt" within a cgraph_edge), and on params of other > functions (e.g. phi-manipulation code). That way we'd avoid the > inconsistency of some accessors using GIMPLE_CHECK and some using > subclasses - all would continue to consistently use GIMPLE_CHECK, but > there would be some extra type-checking and self-documentation of the > expected statement kinds in the code. > > > > FWIW, option 3 is my preferred approach (I've already done the bulk of > the work and it's already been reviewed; it would need an update merger > from trunk, and there's the big gimple to gimple * fixup you wanted). Works for me as well. The compat solution looks somewhat appealing as we can then incrementally fix up things rather than requiring to mass-convert everything. Thanks, Richard. >> Whether you enable them generally or just for selected files >> via a gimple-compat.h will be up to you (but I'd rather get >> rid of them at some point). >> >> Note this allows seamless transform of "random" functions >> taking a gimple now but really only expecting a single kind. >> >> Note that we don't absolutely have to rush this all in for GCC 5. >> Being the very first for GCC 6 stage1 is another possibility. >> We just should get it right. > > Thanks > Dave >