Re: [gomp4] fortran cleanups and c/c++ loop parsing backport

[Cesar Philippidis <cesar at codesourcery dot com>]

On 10/28/2015 04:00 AM, Thomas Schwinge wrote:
> Hi Cesar!
> 
> On Tue, 27 Oct 2015 11:36:10 -0700, Cesar Philippidis <cesar@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  <cesar@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