On 10/28/2015 04:00 AM, Thomas Schwinge wrote: > Hi Cesar! > > On Tue, 27 Oct 2015 11:36:10 -0700, Cesar Philippidis > <ce...@codesourcery.com> 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. Plus, part of those cleanups touched declare, which Jim is going to work on soon. >> In addition, I've also added a couple of more test cases and updated the >> way that combined directives are handled in fortran. Because the >> device_type clauses form a chain of gfc_omp_clauses, I couldn't reuse >> gfc_split_omp_clauses for combined parallel and kernels loops. So that's >> why I introduced a new gfc_filter_oacc_combined_clauses function. > > Thanks, but... > >> I'll apply this patch to gomp-4_0-branch shortly. I know that I should >> have broken this patch down into smaller patches > > Yes. > >> but it was already >> arranged as one big patch in my source tree. > > You're using Git, so that's not a good excuse. :-P I find git to be too temperamental. >> --- a/gcc/fortran/trans-openmp.c >> +++ b/gcc/fortran/trans-openmp.c >> @@ -3634,12 +3634,65 @@ gfc_trans_omp_do (gfc_code *code, gfc_exec_op op, >> stmtblock_t *pblock, >> return gfc_finish_block (&block); >> } >> >> -/* parallel loop and kernels loop. */ >> +/* 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) >> +{ >> + if (*orig_clauses == NULL) >> + { >> + *loop_clauses = NULL; >> + return; >> + } >> + >> + *loop_clauses = gfc_get_omp_clauses (); >> + >> + memset (*loop_clauses, 0, sizeof (gfc_omp_clauses)); > > This has already been created zero-initialized. I was just doing what I was doing before. I removed that in the follow up patch. >> + (*loop_clauses)->gang = (*orig_clauses)->gang; >> + (*orig_clauses)->gang = false; >> + (*loop_clauses)->gang_expr = (*orig_clauses)->gang_expr; >> + (*orig_clauses)->gang_expr = NULL; >> + (*loop_clauses)->gang_static = (*orig_clauses)->gang_static; >> + (*orig_clauses)->gang_static = false; >> + (*loop_clauses)->vector = (*orig_clauses)->vector; >> + (*orig_clauses)->vector = false; >> + (*loop_clauses)->vector_expr = (*orig_clauses)->vector_expr; >> + (*orig_clauses)->vector_expr = NULL; >> + (*loop_clauses)->worker = (*orig_clauses)->worker; >> + (*orig_clauses)->worker = false; >> + (*loop_clauses)->worker_expr = (*orig_clauses)->worker_expr; >> + (*orig_clauses)->worker_expr = NULL; >> + (*loop_clauses)->seq = (*orig_clauses)->seq; >> + (*orig_clauses)->seq = false; >> + (*loop_clauses)->independent = (*orig_clauses)->independent; >> + (*orig_clauses)->independent = false; >> + (*loop_clauses)->par_auto = (*orig_clauses)->par_auto; >> + (*orig_clauses)->par_auto = false; >> + (*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. >> + (*loop_clauses)->tile = (*orig_clauses)->tile; >> + (*orig_clauses)->tile = false; >> + (*loop_clauses)->tile_list = (*orig_clauses)->tile_list; >> + (*orig_clauses)->tile_list = NULL; >> + (*loop_clauses)->device_types = (*orig_clauses)->device_types; >> + >> + gfc_filter_oacc_combined_clauses (&(*orig_clauses)->dtype_clauses, >> + &(*loop_clauses)->dtype_clauses); >> +} >> + >> +/* Combined OpenACC parallel loop and kernels loop. */ >> static tree >> gfc_trans_oacc_combined_directive (gfc_code *code) >> { >> stmtblock_t block, inner, *pblock = NULL; >> - gfc_omp_clauses construct_clauses, loop_clauses; >> + gfc_omp_clauses *loop_clauses; >> tree stmt, oacc_clauses = NULL_TREE; >> enum tree_code construct_code; >> bool scan_nodesc_arrays = false; >> @@ -3661,39 +3714,18 @@ gfc_trans_oacc_combined_directive (gfc_code *code) >> >> gfc_start_block (&block); >> >> - memset (&loop_clauses, 0, sizeof (loop_clauses)); >> - if (code->ext.omp_clauses != NULL) >> - { >> - memcpy (&construct_clauses, code->ext.omp_clauses, >> - sizeof (construct_clauses)); >> - loop_clauses.collapse = construct_clauses.collapse; >> - loop_clauses.gang = construct_clauses.gang; >> - loop_clauses.gang_expr = construct_clauses.gang_expr; >> - loop_clauses.gang_static = construct_clauses.gang_static; >> - loop_clauses.vector = construct_clauses.vector; >> - loop_clauses.vector_expr = construct_clauses.vector_expr; >> - loop_clauses.worker = construct_clauses.worker; >> - loop_clauses.worker_expr = construct_clauses.worker_expr; >> - loop_clauses.seq = construct_clauses.seq; >> - loop_clauses.independent = construct_clauses.independent; >> - construct_clauses.collapse = 0; >> - construct_clauses.gang = false; >> - construct_clauses.vector = false; >> - construct_clauses.worker = false; >> - construct_clauses.seq = false; >> - construct_clauses.independent = false; >> - oacc_clauses = gfc_trans_omp_clauses (&block, &construct_clauses, >> - code->loc); >> - } >> + gfc_filter_oacc_combined_clauses (&code->ext.omp_clauses, &loop_clauses); >> + oacc_clauses = gfc_trans_omp_clauses (&block, code->ext.omp_clauses, >> + code->loc); >> >> array_set = gfc_init_nodesc_arrays (&inner, &oacc_clauses, code, >> scan_nodesc_arrays); >> > > > 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, 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. >> --- 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? Cesar
2015-10-28 Cesar Philippidis <ce...@codesourcery.com> gcc/fortran/ * trans-openmp.c (gfc_filter_oacc_combined_clauses): Don't zero- initialize loop_clauses when it's already initialized. libgomp/ * testsuite/libgomp.oacc-fortran/combined-directive-1.f90: Rename to... * testsuite/libgomp.oacc-fortran/combined-directives-1.f90: ... this. diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c index bec2de4..a4466ed 100644 --- a/gcc/fortran/trans-openmp.c +++ b/gcc/fortran/trans-openmp.c @@ -3651,8 +3651,6 @@ gfc_filter_oacc_combined_clauses (gfc_omp_clauses **orig_clauses, *loop_clauses = gfc_get_omp_clauses (); - memset (*loop_clauses, 0, sizeof (gfc_omp_clauses)); - (*loop_clauses)->gang = (*orig_clauses)->gang; (*orig_clauses)->gang = false; (*loop_clauses)->gang_expr = (*orig_clauses)->gang_expr; @@ -3676,7 +3674,9 @@ gfc_filter_oacc_combined_clauses (gfc_omp_clauses **orig_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. */ + /* Don't reset (*orig_clauses)->collapse. It should be present on + both the kernels/parallel and loop constructs, similar to how + gfc_split_omp_clauses duplicates it in combined OMP constructs. */ (*loop_clauses)->tile = (*orig_clauses)->tile; (*orig_clauses)->tile = false; (*loop_clauses)->tile_list = (*orig_clauses)->tile_list; diff --git a/libgomp/testsuite/libgomp.oacc-fortran/combined-directive-1.f90 b/libgomp/testsuite/libgomp.oacc-fortran/combined-directive-1.f90 deleted file mode 100644 index 94100b2..0000000 --- a/libgomp/testsuite/libgomp.oacc-fortran/combined-directive-1.f90 +++ /dev/null @@ -1,39 +0,0 @@ -! This test exercises combined directives. - -! { dg-do run } - -program main - integer, parameter :: n = 32 - real :: a(n), b(n); - integer :: i - - do i = 1, n - a(i) = 1.0 - b(i) = 0.0 - end do - - !$acc parallel loop copy (a(1:n)) copy (b(1:n)) - do i = 1, n - b(i) = 2.0 - a(i) = a(i) + b(i) - end do - - do i = 1, n - if (a(i) .ne. 3.0) call abort - - if (b(i) .ne. 2.0) call abort - end do - - !$acc kernels loop copy (a(1:n)) copy (b(1:n)) - do i = 1, n - b(i) = 3.0; - a(i) = a(i) + b(i) - end do - - do i = 1, n - if (a(i) .ne. 6.0) call abort - - if (b(i) .ne. 3.0) call abort - end do - -end program main diff --git a/libgomp/testsuite/libgomp.oacc-fortran/combined-directives-1.f90 b/libgomp/testsuite/libgomp.oacc-fortran/combined-directives-1.f90 new file mode 100644 index 0000000..94100b2 --- /dev/null +++ b/libgomp/testsuite/libgomp.oacc-fortran/combined-directives-1.f90 @@ -0,0 +1,39 @@ +! This test exercises combined directives. + +! { dg-do run } + +program main + integer, parameter :: n = 32 + real :: a(n), b(n); + integer :: i + + do i = 1, n + a(i) = 1.0 + b(i) = 0.0 + end do + + !$acc parallel loop copy (a(1:n)) copy (b(1:n)) + do i = 1, n + b(i) = 2.0 + a(i) = a(i) + b(i) + end do + + do i = 1, n + if (a(i) .ne. 3.0) call abort + + if (b(i) .ne. 2.0) call abort + end do + + !$acc kernels loop copy (a(1:n)) copy (b(1:n)) + do i = 1, n + b(i) = 3.0; + a(i) = a(i) + b(i) + end do + + do i = 1, n + if (a(i) .ne. 6.0) call abort + + if (b(i) .ne. 3.0) call abort + end do + +end program main