On Fri, 17 Jul 2015 14:57:14 +0200 Thomas Schwinge <tho...@codesourcery.com> wrote:
> Hi Julian! > > On Thu, 16 Jul 2015 16:32:12 +0100, Julian Brown > <jul...@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