diff mbox

[committed] Fix OpenMP parsing of the specification part in functions (PR fortran/71704)

Message ID 577FDBD1.9050407@codesourcery.com
State New
Headers show

Commit Message

Cesar Philippidis July 8, 2016, 4:58 p.m. UTC 
On 07/08/2016 09:31 AM, Jakub Jelinek wrote:
> On Fri, Jul 08, 2016 at 09:19:01AM -0700, Cesar Philippidis wrote:
>> 2016-07-08  Cesar Philippidis  <cesar@codesourcery.com>
>>
>> 	gcc/fortran/
>> 	* parse.c (matcha): Define.
>> 	(decode_oacc_directive): Add spec_only local var and set it.  Use
>> 	matcha to parse acc data, enter data, exit data, host_data, parallel,
>> 	kernels, update and wait directives.  Return ST_GET_FCN_CHARACTERISTICS
>> 	if a non-declarative directive could be matched.
>>
>> 	gcc/testsuite/
>> 	* gfortran.dg/goacc/pr71704-acc.f90: New test.
> 
> I'd drop the -acc suffix, the directory is enough to differentiate the gomp
> vs. goacc test.

Done.

>> --- a/gcc/fortran/parse.c
>> +++ b/gcc/fortran/parse.c
>> @@ -589,11 +589,25 @@ decode_statement (void)
>>    return ST_NONE;
>>  }
>>  
>> +/* Like match, but don't match anything if not -fopenacc
>> +   and if spec_only, goto do_spec_only without actually matching.  */
> 
> The comment doesn't match what the macro does.  The whole
> decode_oacc_directive function is only called if -fopenacc, so
> it is really "Like a match and if spec_only, ..."

The intent behind that comment was to note it shouldn't be used with the
OpenMP clauses. But I agree that the -fopenacc stuff isn't necessary.
This patch updates the comment.

>> +#define matcha(keyword, subr, st)				\
>> +    do {							\
>> +      if (spec_only && gfc_match (keyword) == MATCH_YES)	\
>> +	goto do_spec_only;					\
>> +      else if (match_word (keyword, subr, &old_locus)		\
>> +	       == MATCH_YES)					\
>> +	return st;						\
>> +      else							\
>> +	undo_new_statement ();				  	\
>> +    } while (0);
>> +
>>  static gfc_statement
>>  decode_oacc_directive (void)
>>  {
>>    locus old_locus;
>>    char c;
>> +  bool spec_only = false;
>>  
>>    gfc_enforce_clean_symbol_state ();
>>  
>> @@ -608,6 +622,10 @@ decode_oacc_directive (void)
>>        return ST_NONE;
>>      }
>>  
>> +  if (gfc_current_state () == COMP_FUNCTION
>> +      && gfc_current_block ()->result->ts.kind == -1)
>> +    spec_only = true;
>> +
>>    gfc_unset_implicit_pure (NULL);
>>  
>>    old_locus = gfc_current_locus;
>> @@ -627,7 +645,7 @@ decode_oacc_directive (void)
>>        match ("cache", gfc_match_oacc_cache, ST_OACC_CACHE);
> 
> Why isn't ST_OACC_ATOMIC matcha?
> At least from the case_executable/case_exec_markers vs.
> case_decl defines, all directives but "routine" and "declare" should
> be matcha IMHO.

Because the atomic directive must operate on a sequence of instructions,
otherwise it should generate a syntax error.

> Also, can you figure out in the OpenACC standard and/or discuss on lang
> committee whether acc declare and/or acc routine can appear anywhere in the
> specification part, or need to be ordered certain way?
> If like in OpenMP they can appear anywhere, then
> case ST_OACC_ROUTINE: case ST_OACC_DECLARE
> should move from case_decl to case_omp_decl macro.

OK, I'll check with them. Can that be a follow up patch, or would you
like to see it resolved in one patch?

Is this patch OK for trunk and gcc6?

Cesar

Comments

Jakub Jelinek July 8, 2016, 5:25 p.m. UTC | #1 
On Fri, Jul 08, 2016 at 09:58:57AM -0700, Cesar Philippidis wrote:
> >> +#define matcha(keyword, subr, st)				\
> >> +    do {							\
> >> +      if (spec_only && gfc_match (keyword) == MATCH_YES)	\
> >> +	goto do_spec_only;					\
> >> +      else if (match_word (keyword, subr, &old_locus)		\
> >> +	       == MATCH_YES)					\
> >> +	return st;						\
> >> +      else							\
> >> +	undo_new_statement ();				  	\
> >> +    } while (0);
> >> +
> >>  static gfc_statement
> >>  decode_oacc_directive (void)
> >>  {
> >>    locus old_locus;
> >>    char c;
> >> +  bool spec_only = false;
> >>  
> >>    gfc_enforce_clean_symbol_state ();
> >>  
> >> @@ -608,6 +622,10 @@ decode_oacc_directive (void)
> >>        return ST_NONE;
> >>      }
> >>  
> >> +  if (gfc_current_state () == COMP_FUNCTION
> >> +      && gfc_current_block ()->result->ts.kind == -1)
> >> +    spec_only = true;
> >> +
> >>    gfc_unset_implicit_pure (NULL);
> >>  
> >>    old_locus = gfc_current_locus;
> >> @@ -627,7 +645,7 @@ decode_oacc_directive (void)
> >>        match ("cache", gfc_match_oacc_cache, ST_OACC_CACHE);
> > 
> > Why isn't ST_OACC_ATOMIC matcha?
> > At least from the case_executable/case_exec_markers vs.
> > case_decl defines, all directives but "routine" and "declare" should
> > be matcha IMHO.
> 
> Because the atomic directive must operate on a sequence of instructions,
> otherwise it should generate a syntax error.

But you are then relying on a nested decode_statement to do something, it
works, but IMHO just rejecting them earlier is much cleaner and more
maintainable, with the simple rule that even can be documented that
declaration directives use match, all others use matcha (similarly how in
decode_omp_directive directives use the matchd[os] while executable directives
use match[os]).
What do you see as advantage of only marking some of the executable
directives?

> > Also, can you figure out in the OpenACC standard and/or discuss on lang
> > committee whether acc declare and/or acc routine can appear anywhere in the
> > specification part, or need to be ordered certain way?
> > If like in OpenMP they can appear anywhere, then
> > case ST_OACC_ROUTINE: case ST_OACC_DECLARE
> > should move from case_decl to case_omp_decl macro.
> 
> OK, I'll check with them. Can that be a follow up patch, or would you
> like to see it resolved in one patch?

Sure, that can be done incrementally.

	Jakub
diff mbox

Patch

2016-07-08  Cesar Philippidis  <cesar@codesourcery.com>

	gcc/fortran/
	* parse.c (matcha): Define.
	(decode_oacc_directive): Add spec_only local var and set it.  Use
	matcha to parse acc data, enter data, exit data, host_data, parallel,
	kernels, update and wait directives.  Return ST_GET_FCN_CHARACTERISTICS
	if a non-declarative directive could be matched.

	gcc/testsuite/
	* gfortran.dg/goacc/pr71704.f90: New test.

diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index d795225..39fdd90 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -589,11 +589,25 @@  decode_statement (void)
   return ST_NONE;
 }
 
+/* Like match and if spec_only, goto do_spec_only without actually
+   matching.  */
+#define matcha(keyword, subr, st)				\
+    do {							\
+      if (spec_only && gfc_match (keyword) == MATCH_YES)	\
+	goto do_spec_only;					\
+      else if (match_word (keyword, subr, &old_locus)		\
+	       == MATCH_YES)					\
+	return st;						\
+      else							\
+	undo_new_statement ();				  	\
+    } while (0);
+
 static gfc_statement
 decode_oacc_directive (void)
 {
   locus old_locus;
   char c;
+  bool spec_only = false;
 
   gfc_enforce_clean_symbol_state ();
 
@@ -608,6 +622,10 @@  decode_oacc_directive (void)
       return ST_NONE;
     }
 
+  if (gfc_current_state () == COMP_FUNCTION
+      && gfc_current_block ()->result->ts.kind == -1)
+    spec_only = true;
+
   gfc_unset_implicit_pure (NULL);
 
   old_locus = gfc_current_locus;
@@ -627,7 +645,7 @@  decode_oacc_directive (void)
       match ("cache", gfc_match_oacc_cache, ST_OACC_CACHE);
       break;
     case 'd':
-      match ("data", gfc_match_oacc_data, ST_OACC_DATA);
+      matcha ("data", gfc_match_oacc_data, ST_OACC_DATA);
       match ("declare", gfc_match_oacc_declare, ST_OACC_DECLARE);
       break;
     case 'e':
@@ -639,19 +657,19 @@  decode_oacc_directive (void)
       match ("end loop", gfc_match_omp_eos, ST_OACC_END_LOOP);
       match ("end parallel loop", gfc_match_omp_eos, ST_OACC_END_PARALLEL_LOOP);
       match ("end parallel", gfc_match_omp_eos, ST_OACC_END_PARALLEL);
-      match ("enter data", gfc_match_oacc_enter_data, ST_OACC_ENTER_DATA);
-      match ("exit data", gfc_match_oacc_exit_data, ST_OACC_EXIT_DATA);
+      matcha ("enter data", gfc_match_oacc_enter_data, ST_OACC_ENTER_DATA);
+      matcha ("exit data", gfc_match_oacc_exit_data, ST_OACC_EXIT_DATA);
       break;
     case 'h':
-      match ("host_data", gfc_match_oacc_host_data, ST_OACC_HOST_DATA);
+      matcha ("host_data", gfc_match_oacc_host_data, ST_OACC_HOST_DATA);
       break;
     case 'p':
       match ("parallel loop", gfc_match_oacc_parallel_loop, ST_OACC_PARALLEL_LOOP);
-      match ("parallel", gfc_match_oacc_parallel, ST_OACC_PARALLEL);
+      matcha ("parallel", gfc_match_oacc_parallel, ST_OACC_PARALLEL);
       break;
     case 'k':
       match ("kernels loop", gfc_match_oacc_kernels_loop, ST_OACC_KERNELS_LOOP);
-      match ("kernels", gfc_match_oacc_kernels, ST_OACC_KERNELS);
+      matcha ("kernels", gfc_match_oacc_kernels, ST_OACC_KERNELS);
       break;
     case 'l':
       match ("loop", gfc_match_oacc_loop, ST_OACC_LOOP);
@@ -660,10 +678,10 @@  decode_oacc_directive (void)
       match ("routine", gfc_match_oacc_routine, ST_OACC_ROUTINE);
       break;
     case 'u':
-      match ("update", gfc_match_oacc_update, ST_OACC_UPDATE);
+      matcha ("update", gfc_match_oacc_update, ST_OACC_UPDATE);
       break;
     case 'w':
-      match ("wait", gfc_match_oacc_wait, ST_OACC_WAIT);
+      matcha ("wait", gfc_match_oacc_wait, ST_OACC_WAIT);
       break;
     }
 
@@ -678,6 +696,13 @@  decode_oacc_directive (void)
   gfc_error_recovery ();
 
   return ST_NONE;
+
+ do_spec_only:
+  reject_statement ();
+  gfc_clear_error ();
+  gfc_buffer_error (false);
+  gfc_current_locus = old_locus;
+  return ST_GET_FCN_CHARACTERISTICS;
 }
 
 /* Like match, but set a flag simd_matched if keyword matched
diff --git a/gcc/testsuite/gfortran.dg/goacc/pr71704.f90 b/gcc/testsuite/gfortran.dg/goacc/pr71704.f90
new file mode 100644
index 0000000..0235e85
--- /dev/null
+++ b/gcc/testsuite/gfortran.dg/goacc/pr71704.f90
@@ -0,0 +1,60 @@ 
+! PR fortran/71704
+! { dg-do compile }
+
+real function f1 ()
+!$acc routine (f1)
+  f1 = 1
+end
+
+real function f2 (a)
+  integer a
+  !$acc enter data copyin(a)
+  f2 = 1
+end
+
+real function f3 (a)
+  integer a
+!$acc enter data copyin(a)
+  f3 = 1
+end
+
+real function f4 ()
+!$acc wait
+  f4 = 1
+end
+
+real function f5 (a)
+  integer a
+!$acc update device(a)
+  f5 = 1
+end
+
+real function f6 ()
+!$acc parallel
+!$acc end parallel
+  f6 = 1
+end
+
+real function f7 ()
+!$acc kernels
+!$acc end kernels
+  f7 = 1
+end
+
+real function f8 ()
+!$acc data
+!$acc end data
+  f8 = 1
+end
+
+real function f9 ()
+!$acc host_data
+!$acc end host_data
+  f8 = 1
+end
+
+real function f10 (a)
+  integer a
+!$acc declare present (a)
+  f8 = 1
+end