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 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. > >> > > >> > I've created a branch "dmalcolm/gimple-classes-v2-option-3" > >> > https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/gimple-classes-v2-option-3 > >> > > >> > which takes the work reviewed by Jeff and the most trivial of the merger > >> > followup work, throwing away the ~80 unloved followup patches on > >> > dmalcolm/gimple-classes. > >> > > >> > I've merged from yesterday's trunk r217593 into that new branch, > >> > resolving conflicts. > >> > > >> > I did this in two parts: the basic merger as > >> > bd7fe714158f0c600caa05be7d744fd9139b8afb > >> > resolving conflicts, with a followup patch to fixup new code from trunk > >> > that used accessors that on the branch required a gimple subclass. > >> > > >> > Attached is that 2nd part of the merger. > >> > > >> > Successfully bootstrapped and regrtested on x86_64-unknown-linux-gnu; > >> > same regrtest results as a control bootstrap of trunk's r217593. > >> > > >> > I appreciate Jakub and others have concerns about the overall approach. > >> > I'm not sure which of option 2 (gimple-compat.h), option 3 (this one), > >> > option 4 (just convert fields and non-accessor params), or defer to gcc > >> > 6 is the best one, but I'm sleep-deprived and wanted to submit this > >> > before the stage1 deadline. > >> > > >> > The other commits on this pruned branch that haven't been reviewed yet > >> > are: > >> > > >> > [gimple-classes, committed 88/92] Preparatory work before subclass > >> > renaming > >> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02820.html > >> > >> Ok. > >> > >> > [gimple-classes, committed 89/92] Eliminate subclass typedefs from > >> > coretypes.h > >> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02838.html > >> > >> Ok. > >> > >> > [gimple-classes, committed 90/92] Automated renaming of gimple > >> > subclasses > >> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02828.html > >> > >> Ok. > >> > >> > [gimple-classes, committed 91/92] Remove out-of-date references to > >> > typedefs] > >> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02874.html > >> > >> Ok. > >> > >> > [gimple-classes, committed 92/92] Update gimple.texi class hierarchy > >> > diagram > >> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02818.html > >> > >> Ok. > >> > >> > [gimple-classes] Merge trunk r216157-r216746 into branch > >> > https://gcc.gnu.org/ml/gcc-patches/2014-10/msg02982.html > >> > >> Ok. > > > > Thanks. You said "Ok" to the various patches I pinged, but I don't > > think you commented on the patch that was attached to the email. > > > > Is that one OK? It's: > > https://gcc.gnu.org/ml/gcc-patches/2014-11/msg01935.html > > in the archives. > > Yes, that one is ok as well. > > > I believe that's the only unreviewed patch on the branch > > "dmalcolm/gimple-classes-v2-option-3": > > https://gcc.gnu.org/git/?p=gcc.git;a=shortlog;h=refs/heads/gimple-classes-v2-option-3 > > > > Assuming that's OK, I want to merge that branch to trunk in the next day > > or so. > > Fine with me.
Thanks. I've merged from today's trunk into the branch, and fixed some whitespace issues Jakub pointed out: https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=cf2cd094c5c77adb40a2f3f69021ee0b6f8534ab (which I assume count as "obvious"), and have bootstrapped®rtested. Am preparing a commit of the branch to svn trunk. Does the gcc/ChangeLog entry need to: (A) contain a full description of the changes being committed relative to trunk (B) contain the body of the ChangeLog.gimple-classes (it's about 4000 lines though) (C) simply contain a pointer back to ChangeLog.gimple-classes ? > >> > 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). > > > > FWIW I got rid of all of those typedefs in the patches above (89/92 in > > particular); the subclasses on the branch are already using explicit > > ptrs, so it's more about consistency between base class ptrs and > > subclass ptrs, to avoid: > > > > gimple stmt; > > gphi *phi; > > gcall *call_stmt; > > > > in favor of: > > gimple *stmt; /* <-- note this one */ > > gphi *phi; > > gcall *call_stmt; > > right. > > > I'd already done that for the subclasses on the branch, and the diff > > between the branch and trunk is relatively sane. > > > > By contrast, doing the gimple -> gimple * fixup will generate a huge > > diff, so that's something to do after merging the branch. > > I'll post the patch here later this week (or a link to it, it's likely > > to be too big even compressed for the ML). > > Thanks, > Richard. > > > Dave > > > >> Thanks, > >> Richard. > >> > >> > Thanks > >> > Dave > >> > [1] e.g. https://gcc.gnu.org/ml/gcc-patches/2014-10/msg01536.html > >> > > >> > > >> >> > 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 > >> >> > > >> > > > > >