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

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 11239 bytes --]

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...


> 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


> --- 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.

> +  (*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.

> +  (*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.


> --- 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.


Additionally, in r229482 I checked in the following to restore bootstrap
builds:

    [...]/source-gcc/gcc/cp/parser.c:29649:1: error: 'tree_node* require_positive_expr(tree, location_t, const char*)' defined but not used [-Werror=unused-function]
     require_positive_expr (tree t, location_t loc, const char *str)
     ^

    [...]/source-gcc/gcc/fortran/openmp.c: In function 'match gfc_match_oacc_declare()':
    [...]/source-gcc/gcc/fortran/openmp.c:1449:30: error: unused variable 'oc' [-Werror=unused-variable]
       gfc_oacc_declare *new_oc, *oc;
                                  ^
    [...]/source-gcc/gcc/fortran/openmp.c: In function 'void gfc_resolve_oacc_declare(gfc_namespace*)':
    [...]/source-gcc/gcc/fortran/openmp.c:4918:7: error: unused variable 'list' [-Werror=unused-variable]
       int list;
           ^

commit 1e53d795ea68bcc7e5d5a7fa21e58c506e557cb4
Author: tschwinge <tschwinge@138bc75d-0d04-0410-961f-82ee72b054a4>
Date:   Wed Oct 28 10:57:45 2015 +0000

    Address -Werror=unused-variable and -Werror=unused-function diagnostics
    
    	gcc/cp/
    	* parser.c (require_positive_expr): Remove function.
    	gcc/fortran/
    	* openmp.c (gfc_match_oacc_declare): Remove oc local variable.
    	(gfc_resolve_oacc_declare): Remove list local variable.
    
    git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@229482 138bc75d-0d04-0410-961f-82ee72b054a4
---
 gcc/cp/ChangeLog.gomp      |    4 ++++
 gcc/cp/parser.c            |   17 -----------------
 gcc/fortran/ChangeLog.gomp |    5 +++++
 gcc/fortran/openmp.c       |    3 +--
 4 files changed, 10 insertions(+), 19 deletions(-)

diff --git gcc/cp/ChangeLog.gomp gcc/cp/ChangeLog.gomp
index c0b96ba..b96bfce 100644
--- gcc/cp/ChangeLog.gomp
+++ gcc/cp/ChangeLog.gomp
@@ -1,3 +1,7 @@
+2015-10-28  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* parser.c (require_positive_expr): Remove function.
+
 2015-10-27  Cesar Philippidis  <cesar@codesourcery.com>
 
 	* parser.c (cp_parser_oacc_shape_clause): Backport from trunk.
diff --git gcc/cp/parser.c gcc/cp/parser.c
index d113cfb..2c6060b 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -29642,23 +29642,6 @@ cp_parser_oacc_data_clause_deviceptr (cp_parser *parser, tree list)
   return list;
 }
 
-/* Attempt to statically determine when the number T isn't positive.
-   Warn if we determined this and return positive one as the new
-   expression.  */
-static tree
-require_positive_expr (tree t, location_t loc, const char *str)
-{
-  tree c = fold_build2_loc (loc, LE_EXPR, boolean_type_node, t,
-			    build_int_cst (TREE_TYPE (t), 0));
-  if (c == boolean_true_node)
-    {
-      warning_at (loc, 0,
-		  "%<%s%> value must be positive", str);
-      t = integer_one_node;
-    }
-  return t;
-}
-
 /* OpenACC:
    num_gangs ( expression )
    num_workers ( expression )
diff --git gcc/fortran/ChangeLog.gomp gcc/fortran/ChangeLog.gomp
index d126367..9e1b95b 100644
--- gcc/fortran/ChangeLog.gomp
+++ gcc/fortran/ChangeLog.gomp
@@ -1,3 +1,8 @@
+2015-10-28  Thomas Schwinge  <thomas@codesourcery.com>
+
+	* openmp.c (gfc_match_oacc_declare): Remove oc local variable.
+	(gfc_resolve_oacc_declare): Remove list local variable.
+
 2015-10-27  Cesar Philippidis  <cesar@codesourcery.com>
 
 	* gfortran.h (gfc_omp_namelist): Add locus where member.
diff --git gcc/fortran/openmp.c gcc/fortran/openmp.c
index 9621eaf..afcce9a 100644
--- gcc/fortran/openmp.c
+++ gcc/fortran/openmp.c
@@ -1446,7 +1446,7 @@ gfc_match_oacc_declare (void)
   gfc_omp_clauses *c;
   gfc_omp_namelist *n;
   gfc_namespace *ns = gfc_current_ns;
-  gfc_oacc_declare *new_oc, *oc;
+  gfc_oacc_declare *new_oc;
   bool module_var = false;
 
   if (gfc_match_omp_clauses (&c, OACC_DECLARE_CLAUSES, 0, false, false, true)
@@ -4915,7 +4915,6 @@ resolve_oacc_declare_map (gfc_oacc_declare *declare, int list)
 void
 gfc_resolve_oacc_declare (gfc_namespace *ns)
 {
-  int list;
   gfc_omp_namelist *n;
   locus loc;
   gfc_oacc_declare *oc;


Grüße
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]