From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 31477 invoked by alias); 19 Nov 2014 10:14:24 -0000 Mailing-List: contact gcc-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-owner@gcc.gnu.org Received: (qmail 31458 invoked by uid 89); 19 Nov 2014 10:14:23 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.1 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW,SPF_PASS autolearn=ham version=3.3.2 X-Spam-User: qpsmtpd, 2 recipients X-HELO: mail-ob0-f171.google.com Received: from mail-ob0-f171.google.com (HELO mail-ob0-f171.google.com) (209.85.214.171) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 19 Nov 2014 10:14:20 +0000 Received: by mail-ob0-f171.google.com with SMTP id uz6so178602obc.30 for ; Wed, 19 Nov 2014 02:14:18 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.202.201.77 with SMTP id z74mr728172oif.70.1416392058751; Wed, 19 Nov 2014 02:14:18 -0800 (PST) Received: by 10.76.101.132 with HTTP; Wed, 19 Nov 2014 02:14:18 -0800 (PST) In-Reply-To: <1416344675.16989.27.camel@surprise> References: <1415373690-26193-1-git-send-email-dmalcolm@redhat.com> <1415373690-26193-5-git-send-email-dmalcolm@redhat.com> <20141108135646.GU5026@tucnak.redhat.com> <1415658470.2209.20.camel@surprise> <20141111072614.GW5026@tucnak.redhat.com> <1415842862.2209.101.camel@surprise> <1416049230.21944.35.camel@surprise> <1416275984.16989.21.camel@surprise> <1416344675.16989.27.camel@surprise> Date: Wed, 19 Nov 2014 10:18:00 -0000 Message-ID: Subject: Re: [gimple-classes, committed 4/6] tree-ssa-tail-merge.c: Use gassign From: Richard Biener To: David Malcolm Cc: Jakub Jelinek , GCC Patches , GCC Development , Jeff Law Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes X-SW-Source: 2014-11/txt/msg00282.txt.bz2 On Tue, Nov 18, 2014 at 10:04 PM, David Malcolm wrote: > On Tue, 2014-11-18 at 10:53 +0100, Richard Biener wrote: >> On Tue, Nov 18, 2014 at 2:59 AM, David Malcolm wro= te: >> > On Mon, 2014-11-17 at 11:06 +0100, Richard Biener wrote: >> >> On Sat, Nov 15, 2014 at 12:00 PM, David Malcolm = wrote: >> >> > On Thu, 2014-11-13 at 11:45 +0100, Richard Biener wrote: >> >> >> On Thu, Nov 13, 2014 at 2:41 AM, David Malcolm wrote: >> >> >> > On Tue, 2014-11-11 at 11:43 +0100, Richard Biener wrote: >> >> >> >> On Tue, Nov 11, 2014 at 8:26 AM, 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 w= rote: >> >> >> >> >> > > 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 =3D as_a (s1); >> >> >> >> >> > > gassign *a2 =3D as_a (s2); >> >> >> >> >> > > lhs1 =3D gimple_assign_lhs (a1); >> >> >> >> >> > > lhs2 =3D gimple_assign_lhs (a2); >> >> >> >> >> > > if (TREE_CODE (lhs1) !=3D SSA_NAME >> >> >> >> >> > > && TREE_CODE (lhs2) !=3D 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) =3D=3D SSA_NAME >> >> >> >> >> > > && TREE_CODE (lhs2) =3D=3D SSA_NAME) >> >> >> >> >> > > return vn_valueize (lhs1) =3D=3D vn_valueize (lh= s2); >> >> >> >> >> > > 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 de= velopers, with no >> >> >> >> >> > gain. You only have to add extra code and extra temporari= es, in switches >> >> >> >> >> > typically also have to add {} because of the temporaries a= nd thus extra >> >> >> >> >> > indentation level, and it doesn't simplify anything in the= code. >> >> >> >> >> >> >> >> >> >> The branch attempts to use the C++ typesystem to capture inf= ormation >> >> >> >> >> 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 ha= s 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 (y= es, 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 manua= lly 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 com= bined, and >> >> >> >> > size of + lines in your patchset? As in, if your changes lea= d to less >> >> >> >> > typing or more. >> >> >> >> >> >> >> >> I see two ways out here. One is to add overloads to all the fu= nctions >> >> >> >> taking the special types like >> >> >> >> >> >> >> >> tree >> >> >> >> gimple_assign_rhs1 (gimple *); >> >> >> >> >> >> >> >> or simply add >> >> >> >> >> >> >> >> gassign *operator ()(gimple *g) { return as_a (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 ever= yone >> >> >> > 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 cla= ss, I >> >> >> > get this error: >> >> >> > >> >> >> > =E2=80=98gassign* operator()(gimple)=E2=80=99 must be a nonstati= c 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, bu= t 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, surel= y). >> >> >> >> >> >> 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 (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 mor= e. >> >> >> > >> >> >> > (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 recent= ly, >> >> >> > 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 tak= ing a >> >> >> > gfoo *, giving a mixture of GIMPLE_CHECK vs subclass use. I be= lieve >> >> >> > the quality of those patches was higher than the later ones on t= he >> >> >> > 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=3Dgcc.git;a=3Dshortlog;h=3Drefs/heads/gi= mple-classes-v2-option-3 >> >> > >> >> > which takes the work reviewed by Jeff and the most trivial of the m= erger >> >> > 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 t= runk >> >> > 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-gn= u; >> >> > same regrtest results as a control bootstrap of trunk's r217593. >> >> > >> >> > I appreciate Jakub and others have concerns about the overall appro= ach. >> >> > I'm not sure which of option 2 (gimple-compat.h), option 3 (this on= e), >> >> > 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=3Dgcc.git;a=3Dshortlog;h=3Drefs/heads/gimpl= e-classes-v2-option-3 >> > >> > Assuming that's OK, I want to merge that branch to trunk in the next d= ay >> > 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=3Dgcc.git;a=3Dcommitdiff;h=3Dcf2cd094c5c77adb4= 0a2f3f69021ee0b6f8534ab > (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 > > ? (A) where I'd mainly document changes to gimple.[ch] and add a boiler-plate * : Update according to the above. (I'm not very fond of gigantic but pretty useless boilerplate changelogs). No need to do (B) (I usually discourage people from doing that) Thanks, Richard. >> >> > Also, presumably if this were merged, it would require a followup w= ith >> >> > the gimple to gimple * fixup you wanted? (which we talked about doi= ng 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 fiel= ds of >> >> >> > structs (e.g. "call_stmt" within a cgraph_edge), and on params o= f 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 o= f the >> >> >> > expected statement kinds in the code. >> >> >> > >> >> >> > >> >> >> > >> >> >> > FWIW, option 3 is my preferred approach (I've already done the b= ulk 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 wan= ted). >> >> >> >> >> >> 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 >> >> >> > >> >> > >> > >> > > >