Joseph, In this rather long reply, I have attempted to collect all your recent feedback, and to provide a response where possible.
JSM: On Mon, 15 Oct 2012, Gary Funck wrote: JSM: GF: Various UPC language related checks and operations JSM: GF: are called in the "C" front-end and middle-end. JSM: GF: To insure that these operations are defined, JSM: GF: when linked with the other language front-ends JSM: GF: and compilers, these functions are stub-ed, JSM: GF: in a fashion similar to Objective C: JSM: JSM: Is there a reason you chose this approach rather than the -fcilkplus JSM: approach of enabling an extension in the C front end given a command-line JSM: option? (If you don't want to support e.g. the ObjC / UPC combination, JSM: you can always give an error in such cases.) In general I think such JSM: conditionals are preferable to linking in stub variants of functions - and JSM: I'm sure people doing all-languages LTO bootstraps will appreciate not JSM: having to do link-time optimization of the language-independent parts of JSM: the compiler yet more times because of yet another binary like cc1, JSM: cc1plus, ... that links in much the same code. The functions you stub out JSM: would then all start with assertions that they are only ever called in UPC JSM: mode - or if they are meant to be called in C mode but do nothing in that JSM: case, with appropriate checks that return early for C (if needed). JSM: JSM: On Mon, 15 Oct 2012, Gary Funck wrote: JSM: GF: Back when we began to develop GUPC, it was recommended that we JSM: GF: introduce the UPC capability as a language dialect, similar to JSM: GF: Objective C. That is the approach that we have taken. JSM: JSM: Recommended where? I think that approach has been a bad idea for a long JSM: time and the approach of building into cc1, as taken by the cilkplus JSM: patches, is better (and that really most objc/ code should be like JSM: c-family/, built once and linked into both cc1 and cc1plus, though in its JSM: present state that's much harder to achieve). JSM: JSM: GF: I agree that there is no de facto reason that cc1upc is built JSM: GF: other than the fact we use a similar approach to Objective C. JSM: GF: However, I think that re-working this aspect of how GUPC is JSM: GF: implemented will require a fair amount of time/effort. If we JSM: JSM: I'd expect it to be a fairly straightforward rework (as would making ObjC JSM: a mode of cc1, if ObjC++ didn't exist). GUPC (then called GCC/UPC) dates back to the GCC 2.7 and GCC 2.95 days. The GCC 2.7 based implementation was a prototype, and the GCC 2.95 version was a first attempt to implement a UPC compiler that fits within the GCC build framework. At the time, we had discussions with Mark Mitchell, Mike Stump and perhaps others regarding the best method for introducing a UPC compiler into GCC. The consensus recommendation at the time was to implement GCC/UPC as a language dialect, similar to Objective-C. Thus, much of the initial work was patterned after ObjC; it used stubs and built a distinct front-end, so that is what we did. I will note that even back in those days that Mark indicated he didn't particularly like the language dialect approach, but it seemed the best way to go at the time. For Clang UPC, we have built the UPC capability into the Clang C/C++ compiler. This just turned out to be an easier way to go given Clang's build structure. My main issue with entertaining large re-structuring of GUPC at the moment is that I would like to see if we can introduce GUPC into GCC 4.8. As you point out, there are a lot of other things that we need to do to align the GUPC changes with the GCC trunk. Ideally, I'd like to see if we can phase those changes; dealing with the critical changes now prior to the merge and then implementing the rest of the changes over time. Is that feasible or likely? JSM: GF: Some UPC-specific configuration options are added to JSM: GF: the top-level configure.ac and gcc/configure.ac scripts. JSM: JSM: Any patch that includes new configure options should include the JSM: documentation for those options in install.texi. Also please include JSM: ChangeLog entries with each patch submission, and a more detailed JSM: description of the changes involved and the rationale for them and JSM: implementation choices made. OK, thanks. JSM: The changes to darwin.c, darwin.h, i386.c, rs6000.c should definitely be JSM: given their own rationale and called to the attention of relevant target JSM: maintainers, possibly through extracting them into separate patches in the JSM: series with meaningful subject lines mentioning the relevant targets, not JSM: mixed into what's ostensibly a build-system patch. OK. JSM: The configure options need justification for their existence and (given JSM: that they exist) for working using #if, given that (as I said before, in JSM: the above referenced 2010 message, in JSM: <http://gcc.gnu.org/ml/gcc-patches/2011-07/msg00083.htmlGF: and JSM: <http://gcc.gnu.org/ml/gcc-patches/2011-07/msg00135.html>), "if" JSM: conditionals are preferred to #if and it's better for users to be able to JSM: choose things when they run the compiler rather than only having it JSM: determined by the person building the compiler (so configure options JSM: determining defaults are better than configure options that force the JSM: compiler to behave in a particular way - even if the option affects JSM: runtime library builds, it's still better to have a command-line option JSM: and the configure option just affect the default, since then at least JSM: people can see how code generation changes with the option and JSM: potentially build multilibs with both variants). I agree with your overall recommendation, but found it difficult to implement the various flavors of UPC runtime libraries within the GCC build framework. I may post a follow-up question/two on the issue of building various libgupc "flavors". I wasn't certain that multilib was the way to go for this. Is the regular GCC list the best place for that type of discussion? Also, specifying the flavor of PTS representation on the UPC compiler command line (for example) is a departure from the current method of configuring and invoking GUPC. There is a ripple effect of documentation, testing, and user education that would have to occur to make this change. For Clang UPC we do support specification of the PTS rep. on the compiler command line, and provide a suitable default; the Clang build infrastructure was easier to work with in this regard. Although an implementation detail, in the long run we would like to unify the PTS representations and simplify the configuration variants, but that work needs to be prioritized along with everything else. JSM: In general, please make sure that you take account of previous feedback on JSM: previous GUPC patch submissions, and explain in your submissions how you JSM: have adjusted things for this feedback, or, if you are not making a JSM: previously requested change, why you consider the approach taken in your JSM: patch to be optimal. As mentioned in a previous reply, we did attempt to incorporate previous feedback into GUPC, and we do appreciate all feedback/suggestions and try to make all suggested updates. However, it seems that we missed a few of them. We'll try to do better. JSM: The change to ACX_BUGURL in configure.ac is simply wrong. JSM: JSM: Adding $(C_STUB_OBJS) uses to random language makefiles is very JSM: suspicious. stub-objc.o isn't needed by non-C-family front ends. Nor JSM: should stub-upc.o be - although I think the whole approach of using stub JSM: functions like that is wrong, as discussed. I'm not fond of stubs or adding extra files to other front-ends, but those changes seemed necessary at the time to get things to link. BTW, regarding something like UPC++: there is some interest but there are some language issues that would need to be addressed. JSM: The language-independent JSM: compiler should be using language-independent interfaces to process JSM: language-independent internal representations; if there are additions to JSM: the language-independent IR to handle UPC, then the relevant code to JSM: handle them should be built in unconditionally. We followed the Objective C approach for better/worse. The few additional tree nodes needed for UPC are defined in a language dependent tree definition file. The UPC-specific support needed by the "C" front-end are defined in the 'upc' directory in a fashion similar to the 'objc' directory. When UPC isn't being built, those functions are stubbed. Thus, generally there is little/no UPC-related #if (conditional) code in the "C" front-end. As far as the language-independent parts go, I can think of one/two places that we might still use conditionals. I will point those out in the next set of review notes. JSM: Front ends should not have their own Makefile.in if at all avoidable, only JSM: Make-lang.in - why do you have gcc/upc/Makefile.in? Note that it says JSM: "Derived from objc/Makefile.in", which was removed in r37088 (2000-10-27). This is an oversight. That Makefile.in dates back to the days that we copied the files from the objc directory. It is unused and will be removed. JSM: See what I said in my 2010 review about "careful review for whether it ... JSM: takes account of cleanups done in the past decade or so" - you really do JSM: need to read through the entirety of the code to be submitted, line by JSM: line, and check how it stands up to the conventions followed elsewhere in JSM: the compiler and to issues that get raised in reviews of other patches JSM: nowadays. (But with rearrangement to be an option to the C front end, I JSM: expect you shouldn't need upc/Make-lang.in either.) We haven't been involved in the re-structuring of the GCC infra-structure, but do try to adjust to changes when we merge with the trunk. From my point of view, the recommended best practice has evolved over the years. For example, the recommendation to select UPC compilation via a compiler command line switch rather than to build cc1upc in a fashion similar to Objective C as it is done now, wasn't mentioned in either the 2010 or 2011 review comments. I can see the benefit of all your recommendations. I am just trying to see if there is a way to phase them so that we can merge into GCC 4.8? I was also hoping to get to the point that we could have some review of the non-infrastructure related changes (such as the "C" front-end and other changes) in parallel so that we can better plan our work. JSM: Please go back to my previous comments, then post a full patch series JSM: *once those issues are addressed*. Each submission needs ChangeLog JSM: entries and more detailed descriptions / rationale. OK. JSM: For example, explain JSM: why a lang hook change is made in c-objc-common.h. Make sure that you JSM: consistently use error_at, not error, for new diagnostics, or explain in JSM: your rationale why a location isn't readily available. Make sure that JSM: source lines do not go over 80 columns. There are at least some other JSM: coding standards issues; some visible at a glance are the comment /* for JSM: static declarations of shared arrays */ (start with a capital letter, end JSM: with '.', actually say what the semantics of this variable's value are if JSM: it needs a comment) and a missing space in TREE_CODE(decl). OK. Generally, we do try to follow the GCC coding standards, but it seems that a few variances remain. We will address those. JSM: Avoid any diff lines that are solely changing trailing whitespace (you JSM: have at least once diff hunk in c-decl.c that appears to do nothing but JSM: add such whitespace). Some indentation also looks suspicious - look at JSM: the @@ -8859,6 +9048,23 @@ declspecs_add_qual hunk, for example, where JSM: inconsistent indentation reveals that you are indenting with spaces in JSM: some places, except for one line using a TAB (all new lines should be JSM: indented with TABs for every 8 spaces). OK. JSM: On Mon, 15 Oct 2012, Gary Funck wrote: JSM: GF: Regarding ChangeLog entries, I can add them to each JSM: GF: change set, if that is needed. However, I was hoping to JSM: GF: wait on that until the patches are generally approved, JSM: GF: in principle. JSM: JSM: As a patch submitter, it's your responsibility to make it easy for JSM: reviewers to review your patch. The ChangeLogs provide a roadmap for the JSM: patch - showing what is changed where, at a glance. They go alongside the JSM: plain text rationale, explaining at a higher level the structure and JSM: rationale for the changes and anything that might seem unclear as to why JSM: the patches work in a particular way. JSM: JSM: Each patch submission's Subject: line should best include a brief JSM: description of that patch as well, not just a patch number. OK. JSM: Any changes that are not immediately and obviously inherently UPC-specific JSM: deserve specific explanation in the plain text rationale. That certainly JSM: includes such things as the langhook change I mentioned, the target JSM: changes and the changes to non-C-family front-end Make-lang.in files. In JSM: the case of the target changes, there should be a high-level explanation JSM: of how other target maintainers might determine whether changes to their JSM: targets might be appropriate for UPC, or how you determined that only JSM: those targets should be changed. OK. JSM: For changes developed over several years, reading through them in detail JSM: to prepare a ChangeLog is particularly valuable as it will show up places JSM: where there are spurious changes (such as those to whitespace) that may JSM: have resulted from code being changed and changed again in the course of JSM: development, but that are not needed for a clean patch submission. JSM: JSM: I don't really think your division into 16 patches is a particularly good JSM: one - it separates things that should be together, and joins things that JSM: might better be separate. Putting documentation in a separate patch from JSM: the things documented is always bad, for example - if you have new target JSM: hooks, put the .texi changes in the same patch that adds those hooks. A JSM: better division might be: JSM: JSM: * Target changes, split out per target. JSM: JSM: * Changes to existing C front-end files (including c-family). JSM: JSM: * Changes to any other front ends, split out by front end. JSM: JSM: * New front-end files. JSM: JSM: * Changes to the language and target independent compiler (including build JSM: system code). JSM: JSM: * Library. JSM: JSM: * Compiler testsuite. OK, I will re-package the patches, add ChangeLog's, review the changes more carefully and provide a rationale per your recommendations. I appreciate the detailed feedback that you have provided. thanks, - Gary
