Hi! On Wed, 28 Oct 2015 08:30:56 -0700, Cesar Philippidis wrote: > On 10/28/2015 04:00 AM, Thomas Schwinge wrote: > > On Tue, 27 Oct 2015 11:36:10 -0700, Cesar Philippidis wrote: > >> This patch contains the following: > >> > >> * C front end changes from trunk: > >> https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02528.html > >> > >> * C++ front end changes from trunk: > >> https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02540.html > >> > >> * Proposed fortran cleanups and enhanced error reporting changes: > >> https://gcc.gnu.org/ml/gcc-patches/2015-10/msg02288.html > > > > I suppose the latter is a prerequisite for other Fortran front end > > changes you've also committed? Otherwise, why not get that patch into > > trunk first? That sould save me from having to deal with potentially > > more merge conflicts later on... > > It wasn't necessarily a prerequisite for these changes, but I've been > trying to get that patch into trunk for a while now. Generally, please get such "widespread" patches (touching a lot of code, even if trivially) into trunk first. With every merge from trunk into gomp-4_0-branch I'll now have to make sure to adjust every new trunk code to cooperate with your patch. Jakub, can you please review ? > Plus, part of those > cleanups touched declare, which Jim is going to work on soon. I don't understand how that relates. Anyway, the patch is now committed on gomp-4_0-branch, and hopefully will land in trunk soon; let's move on. > >> --- a/gcc/fortran/trans-openmp.c > >> +++ b/gcc/fortran/trans-openmp.c > >> +/* Helper function to filter combined oacc constructs. ORIG_CLAUSES > >> + contains the unfiltered list of clauses. LOOP_CLAUSES corresponds to > >> + the filter list of loop clauses corresponding to the enclosed list. > >> + This function is called recursively to handle device_type clauses. */ > >> + > >> +static void > >> +gfc_filter_oacc_combined_clauses (gfc_omp_clauses **orig_clauses, > >> + gfc_omp_clauses **loop_clauses) > >> + (*loop_clauses)->acc_collapse = (*orig_clauses)->acc_collapse; > >> + (*orig_clauses)->acc_collapse = false; > >> + (*loop_clauses)->collapse = (*orig_clauses)->collapse; > >> + /* Don't reset (*orig_clauses)->collapse. */ > > > > Why? (Extend source code comment?) The original code (cited just below) > > did this differently. > > Because that's what gfc_split_omp_clauses does. I'm not sure what that's > required for gfc_trans_omp_do, but it is. gfc_trans_omp_do appears to be > operating on two sets of clauses for some non-obvious reason. > > >> @@ -3661,39 +3714,18 @@ gfc_trans_oacc_combined_directive (gfc_code *code) > >> - loop_clauses.collapse = construct_clauses.collapse; > >> - [...] > >> - construct_clauses.collapse = 0; (The "old" behavior is also what the C/C++ front ends are doing, by the way, so that would also need to be updated.) Assuming I'm looking at the correct thing, the collapse clause handling in gfc_split_omp_clauses says "duplicate collapse" which is more expressive to me than "don't reset (*orig_clauses)->collapse". I haven't researched the requirement for OpenMP, but I don't see how a collapse clause would be relevant to have on an OpenACC parallel or kernels construct? Which detail do I overlook? > > With... > > > >> --- /dev/null > >> +++ b/gcc/testsuite/gfortran.dg/goacc/combined-directives.f90 > > > > ... newly added, and... > > > >> --- a/libgomp/testsuite/libgomp.oacc-fortran/combdir-1.f90 > >> +++ /dev/null > > > > ... renamed to... > > > >> --- /dev/null > >> +++ b/libgomp/testsuite/libgomp.oacc-fortran/combined-directive-1.f90 > > > > ... this, plus the unaltered > > libgomp/testsuite/libgomp.oacc-c-c++-common/combdir-1.c, we now got three > > variants: "combined-directives", "combined-directive", and "combdir". > > Please settle on one of them. > > The problem here was I didn't know what compdir stood for *Comb*ined *dir*ectives, I guessed. > so I renamed > it to combined-directive-foo. I guess I didn't keep the name consistent > when I introduced a new compiler time test. The attached patch adjusts > the test in libgomp to make it consistent with it's compile time test. Again, to not make merges needlessly difficult, the preference is to do such changes on trunk first. (Which I'll now do in the following, also taking care to rename libgomp/testsuite/libgomp.oacc-c-c++-common/combdir-1.c in the same way...). > >> --- a/gcc/testsuite/gfortran.dg/gomp/intentin1.f90 > >> +++ b/gcc/testsuite/gfortran.dg/gomp/intentin1.f90 > >> @@ -11,6 +11,6 @@ subroutine foo (x) > >> !$omp simd linear (x) ! { dg-error "INTENT.IN. POINTER" } > >> do i = 1, 10 > >> end do > >> -!$omp single ! { dg-error "INTENT.IN. POINTER" } > >> -!$omp end single copyprivate (x) > >> +!$omp single > >> +!$omp end single copyprivate (x) ! { dg-error "INTENT.IN. POINTER" } > >> end > > > > Please revert this unrelated change. > > This is related to the fortran patch I referenced above. That fortran > patch did two things: > > 1. It introduced a function to detect if variables appear in in > omp map lists. (Because we don't support 'acc copyin (foo) > copyout (foo)') > > 2. It associates a gfc_locus with each entry inside an omp map list. > That's why I had to adjust that test case. > > I could revert that patch from gomp4, but it really impacts declare. > There's so much copy-and-paste going on there. What do you want to do > with this? Copy-and-paste? Anyway, this is my fault: it looked to me like just whitespace change, and I didn't realize you'd moved the dg-error directive from one line to another; all good then. Grüße Thomas