Re: [gomp4] Remove device-specific filtering during parsing for OpenACC

[Julian Brown <julian at codesourcery dot com>]

On Fri, 17 Jul 2015 14:57:14 +0200
Thomas Schwinge <thomas@codesourcery.com> wrote:

> Hi Julian!
> 
> On Thu, 16 Jul 2015 16:32:12 +0100, Julian Brown
> <julian@codesourcery.com> wrote:
> > This patch removes the device-specific filtering (for NVidia PTX)
> > from the parsing stages of the host compiler (for the device_type
> > clause -- separately for C, C++ and Fortran) in favour of fully
> > parsing the device_type clauses, but not actually implementing
> > anything for them (device_type support is a feature that we're not
> > planning to implement just yet: the existing "support" is something
> > of a red herring).
> > 
> > With this patch, the parsed device_type clauses will be ready at OMP
> > lowering time whenever we choose to do something with them (e.g.
> > transforming them into a representation that can be streamed out and
> > re-read by the appropriate offload compiler). The representation is
> > more-or-less the same for all supported languages
> 
> Thanks!
> 
> > modulo clause ordering.
> 
> Is that something that a) doesn't need to be/already has been
> addressed (with your patch), or b) still needs to be addressed?

It's something that doesn't matter, I think: clauses are chained
together like this:

  num_gangs
  num_workers
  ...
  |
  device_type(foo)
  \__num_gangs    (OMP_CLAUSE_DEVICE_TYPE_CLAUSES)
  |  num_workers
  |  ...
  device_type(bar)
  \__num_gangs
  |  num_workers
  |  ...
  V
  (OMP_CLAUSE_CHAIN)

"foo" and "bar" are OMP_CLAUSE_DEVICE_TYPE_DEVICES -- tree lists. The
Fortran front-end will emit num_gangs, num_workers etc. clauses in a
fixed order (irrespective of their order in the source program), but the
C and C++ frontends will emit them in the (reverse of the) order
encountered.

There isn't really a consumer for this information yet, but when there
is, it will just have to not care about that (which should be
straightforward, I think).

> > I've altered the dtype-*.* tests to account for the new behaviour
> > (and to not use e.g. mixed-case "nVidia" or "acc_device_nvidia"
> > names, which are contrary to the recommendations in the spec).
> 
> OpenACC 2.0a indeed seems to suggest that device_type arguments are
> case-sensitive -- contrary to the ACC_DEVICE_TYPE environment
> variable, which probably is where the idea came from to parse them
> case-insensitive.
> 
> As to the latter "invalid" names, I thought the idea has been to
> verify that the clauses following such device_types clauses are
> indeed ignored in the later processing.  (Obviously, there should've
> been comments indicating that, as otherwise that's very confusing --
> as we've just seen -- due to the similarity to the runtime library's
> acc_device_* device type values.)

Yes, and there are still some tests for that functionality. I figured
there wasn't much point in "over-testing" it, especially since none of
this code does that much yet.

> > OK to apply, or any comments?
> 
> Your commit r225927 appears to have caused:
> 
>     [-PASS:-]{+FAIL: libgomp.fortran/declare-simd-2.f90   -O0
> (internal compiler error)+} {+FAIL:+}
> libgomp.fortran/declare-simd-2.f90   -O0  (test for excess errors)
> [-PASS:-]{+UNRESOLVED:+} libgomp.fortran/declare-simd-2.f90   -O0
> [-execution test-] [-PASS:-]{+compilation failed to produce
> executable+} [same for other optimization levels]
> 
>     [...]/source-gcc/libgomp/testsuite/libgomp.fortran/declare-simd-3.f90:17:0:
> internal compiler error: Segmentation fault 0xc39b6f crash_signal
>             [...]/source-gcc/gcc/toplev.c:352
>     0x7043a8 gfc_trans_omp_clauses
>             [...]/source-gcc/gcc/fortran/trans-openmp.c:2671
>     0x7049a8 gfc_trans_omp_declare_simd(gfc_namespace*)
>             [...]/source-gcc/gcc/fortran/trans-openmp.c:4589
>     0x6b8542 gfc_get_extern_function_decl(gfc_symbol*)
>             [...]/source-gcc/gcc/fortran/trans-decl.c:2025
>     0x6b878d gfc_get_extern_function_decl(gfc_symbol*)
>             [...]/source-gcc/gcc/fortran/trans-decl.c:1820
>     0x6ce952 conv_function_val
>             [...]/source-gcc/gcc/fortran/trans-expr.c:3601
>     0x6ce952 gfc_conv_procedure_call(gfc_se*, gfc_symbol*,
> gfc_actual_arglist*, gfc_expr*, vec<tree_node*, va_gc, vl_embed>*)
> [...]/source-gcc/gcc/fortran/trans-expr.c:5873 0x6cf4c2
> gfc_conv_expr(gfc_se*, gfc_expr*)
> [...]/source-gcc/gcc/fortran/trans-expr.c:7391 0x6d71d0
> gfc_trans_assignment_1 [...]/source-gcc/gcc/fortran/trans-expr.c:9127
>     0x692465 trans_code
>             [...]/source-gcc/gcc/fortran/trans.c:1674
>     0x6fa457 gfc_trans_omp_code
>             [...]/source-gcc/gcc/fortran/trans-openmp.c:2711
>     0x705410 gfc_trans_omp_do
>             [...]/source-gcc/gcc/fortran/trans-openmp.c:3459
>     0x707f9f gfc_trans_omp_directive(gfc_code*)
>             [...]/source-gcc/gcc/fortran/trans-openmp.c:4521
>     0x6922b7 trans_code
>             [...]/source-gcc/gcc/fortran/trans.c:1924
>     0x6c0660 gfc_generate_function_code(gfc_namespace*)
>             [...]/source-gcc/gcc/fortran/trans-decl.c:6231
>     0x64d630 translate_all_program_units
>             [...]/source-gcc/gcc/fortran/parse.c:5523
>     0x64d630 gfc_parse_file()
>             [...]/source-gcc/gcc/fortran/parse.c:5728
>     0x68ef12 gfc_be_parse_file
>             [...]/source-gcc/gcc/fortran/f95-lang.c:214

Thanks, I'll have a look at this.

> > --- a/gcc/c/c-parser.c
> > +++ b/gcc/c/c-parser.c
> > @@ -12439,10 +12439,7 @@ c_parser_oacc_all_clauses (c_parser
> > *parser, omp_clause_mask mask, c_parser_skip_to_pragma_eol (parser);
> >  
> >    if (finish_p)
> > -    {
> > -      clauses = c_oacc_filter_device_types (clauses);
> > -      return c_finish_omp_clauses (clauses, true);
> > -    }
> > +    return c_finish_omp_clauses (clauses, true);
> >  
> >    return clauses;
> >  }
> 
> In combination with the equivant change to
> gcc/cp/parser.c:cp_parser_oacc_all_clauses,
> gcc/c-family/c-omp.c:c_oacc_filter_device_types, and transitively also
> the struct identifier_hasher and c_oacc_extract_device_id function
> preceding it, are now unused.  (Not an exhaustive list; have not
> checked which other auxilliary functions etc. Cesar has added in his
> device_type changes.)  Does it make any sense to keep these for
> later, or dump them now?

It probably makes sense to dump them.

> > --- a/gcc/c/c-typeck.c
> > +++ b/gcc/c/c-typeck.c
> > @@ -12568,6 +12568,10 @@ c_finish_omp_clauses (tree clauses, bool
> > oacc) pc = &OMP_CLAUSE_CHAIN (c);
> >  	  continue;
> >  
> > +        case OMP_CLAUSE_DEVICE_TYPE:
> > +	  pc = &OMP_CLAUSE_DEVICE_TYPE_CLAUSES (c);
> > +	  continue;
> > +
> >  	case OMP_CLAUSE_INBRANCH:
> >  	case OMP_CLAUSE_NOTINBRANCH:
> >  	  if (branch_seen)
> 
> From a quick glance only, this seems to be different from the C++
> front end (have not checked Fortran).
> 
> I have not looked at what the front end parsing is now actually
> doing; is it just attaching any clauses following a device_type
> clause to the latter?  (The same should be done for all front ends,
> obviously.  Even if it's not important right now, because of the
> sorry diagnostic that will be emitted later on as soon as there is
> one device_type clause, this should best be addressed now, while you
> still remember what's going on here ;-) so that there will be no bad
> surprises once we actually implement the handling in OMP
> lowering/streaming/device compilers.)
> 
> Do we need manually need to take care to
> "finalize" (c_finish_omp_clauses et al.) such "masked" clause chains,
> or will the right thing happen automatically?
> 
> For reference, C++ does not appear to use
> OMP_CLAUSE_DEVICE_TYPE_CLAUSES here:
> 
> > --- a/gcc/cp/pt.c
> > +++ b/gcc/cp/pt.c
> > @@ -13666,6 +13666,7 @@ tsubst_omp_clauses (tree clauses, bool
> > declare_simd, case OMP_CLAUSE_AUTO:
> >  	case OMP_CLAUSE_SEQ:
> >  	case OMP_CLAUSE_TILE:
> > +	case OMP_CLAUSE_DEVICE_TYPE:
> >  	  break;
> >  	default:
> >  	  gcc_unreachable ();
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -5951,6 +5951,7 @@ finish_omp_clauses (tree clauses, bool oacc)
> >  	case OMP_CLAUSE_BIND:
> >  	case OMP_CLAUSE_NOHOST:
> >  	case OMP_CLAUSE_TILE:
> > +	case OMP_CLAUSE_DEVICE_TYPE:
> >  	  break;
> >  
> >  	case OMP_CLAUSE_INBRANCH:
> 
> (I have not checked Fortran.)

Hmm, not sure about that.

> I also remember that I had a comment regarding device_type handling in
> gcc/c-family/c-omp.c:c_oacc_split_loop_clauses,
> <http://news.gmane.org/find-root.php?message_id=%3C87zj2z2e4x.fsf%40schwinge.name%3E>.
> Given that these clauses are now no longer being handled to
> completion in the front ends, does this need to be addressed?

Nor that.

> > --- a/gcc/omp-low.c
> > +++ b/gcc/omp-low.c
> > @@ -2028,6 +2028,7 @@ scan_sharing_clauses (tree clauses,
> > omp_context *ctx) case OMP_CLAUSE_AUTO:
> >  	case OMP_CLAUSE_SEQ:
> >  	case OMP_CLAUSE_TILE:
> > +	case OMP_CLAUSE_DEVICE_TYPE:
> >  	  break;
> >  
> >  	case OMP_CLAUSE_ALIGNED:
> > @@ -2163,6 +2164,7 @@ scan_sharing_clauses (tree clauses,
> > omp_context *ctx) case OMP_CLAUSE_AUTO:
> >  	case OMP_CLAUSE_SEQ:
> >  	case OMP_CLAUSE_TILE:
> > +	case OMP_CLAUSE_DEVICE_TYPE:
> >  	  break;
> >  
> >  	case OMP_CLAUSE_DEVICE_RESIDENT:
> > @@ -9774,6 +9776,10 @@ expand_omp_target (struct omp_region *region)
> >  	tree t_async;
> >  	int t_wait_idx;
> >  
> > +	c = find_omp_clause (clauses, OMP_CLAUSE_DEVICE_TYPE);
> > +	if (c)
> > +	  sorry ("device_type clause is not supported yet");
> > +
> >  	/* Default values for t_async.  */
> >  	t_async = fold_convert_loc (gimple_location (entry_stmt),
> >  				    integer_type_node,
> 
> Typically, sorry messages are emitted in the generic clause handling
> code in scan_sharing_clauses.

OK, I guess that can be moved in a follow-up patch.

Cheers,

Julian