Re: RFC: Merge the GUPC branch into the GCC 4.8 trunk (patch 01 of 16)

[Gary Funck <gary at intrepid dot com>]

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