RE: Initial submission of OpenACC support integrated into OpenMP's lowering and expansion passes

[Evgeny Gavrin <e dot gavrin at samsung dot com>]

Hi, Thomas!

This is really great!
I've looked at your changes and in most of front-end parts it looks reasonable to me. As you know, we're like-minded with you about how OpenACC's front-ends should look like. So, I think it's good that you have working flow for your implementation. Now we can start to merge front-ends from openacc-1_0-branch to GOMP, while keeping all understandings of ACC in sync and working.

Jakub, don't you mind if I prepare a few front-end patches likewise for review to extend Thomas' vision with the things that was already done by my colleagues on the ACC branch? I hope it'll much speed up the development.

> the last patch adds GOACC_parallel, which so far simply branches to 
> GOMP_target.  There's more to come.
I've reviewed ACC-related code from [openacc-1_0-branch] and from the patches, I noticed there are some mess around marking OpenACC specific code. There are ACC, GACC, OACC, GOACC abbreviations used across code, which one is the best? I prefer ACC, this is not critical, I'm just curious how to resolve this.

> Due to the conceptual similarity compared to OpenMP, and that (later) 
> it is reasonable to expect to embed OpenACC directives into OpenMP 
> ones, the approach I've chosen is to directly embed OpenACC handling 
> into the existing OpenMP infrastructure/passes.
Regarding front-ends, not only from view of OMP/ACC, but front-ends itself, it makes sense to move some parsing routines out of gomp/acc context. This will be useful to implement not only ACC, but also HMPP, if someone is interested, or other similar technology. And of cause, these functions should be used behind the existing facilities - not to break current implementation and other dependencies. So it won't not introduce any difficulties for back porting, while generalizing front-ends a bit.
For now, I prefer to extend the approved way of developing, but keep it in mind, please.

> This patch series doesn't contain any substantial rumtime library work 
> yet;
Can you share some technical details/ideas about further implementation, it's not going to be trivial. On the [openacc-1_0-branch] we resolved it with OpenCL-driven runtime. Also, OpenACC 2.0 standard declares some vendor-specific routines, this should be noted too.

> This is in contrast to Samsung's work, who are implementing OpenACC
> separately from the existing OpenMP support.
I'm not fully agree that this in contrast to things are taking place in [openacc-1_0-branch]. The main reason for keeping middle-end and back-end solutions far from GOMP is the understanding of OpenACC semantics and unclear understanding of what should be done to generalize support of accelerators in GCC. OpenACC and OpenOMP has some semantically differences, but such things matters only for middle-end and further code generation. The main one, is the memory concept. The 

> Yet, I hope we'll be able to share/re-use at least front end and some
middle end code.
I absolutely agree, it's needed to merge our efforts regarding front-ends.
Many things from ACC in [openacc-1_0-branch] is already done, so it doesn't necessary to re-implement same things. Maybe we can polish front-ends together and then commit changes to GOMP?

> We directly strive for OpenACC 2.0 support, skipping OpenACC 1.  We're 
> focussing on the C front end implementation first, following on with 
> C++ and Fortran later on.
How do you think, does it make sense to adopt current Fortran implementation from the OpenACC branch? This can be done quite soon. Also, I may help to extend C and C++ front-end the same way it's done now, by adding other implemented directives, too.

Regarding proposed patches:
[8/9] c_parser_omp_all_clauses and cp_parser_omp_all_clauses
-	  c_parser_error (parser, "expected %<#pragma omp%> clause");
+	  c_parser_error (parser, "expected clause");

-	  cp_parser_error (parser, "expected %<#pragma omp%> clause");
+	  cp_parser_error (parser, "expected clause");
Does it really needed to remove %<#pragma omp%> in these cases? We handle both, I think.

[9/9]
* gimple.h (gimple_build_oacc_*): New declaration.
gimple_oacc_parallel_* and other functions like that. Let's move these functions out of gimple.* to gimple-oacc.*! There are going to be much more of the stuff like this, maybe it'll make sense to keep it externally? May be not only gimple manipulation functions but also pretty-print Also, have a look on openacc_1-0_branch please, for gimple-oacc.*, is OK if I'll add some of this?

Handling clauses.
+  GIMPLE_CHECK (gs, GIMPLE_OACC_PARALLEL);  
+ gs->gimple_omp_parallel.clauses = clauses;
I think, we can freely add some kind of flag to gimple_omp_parallel to indicate that we're are working with ACC.
Or even add new gimple_acc_* statements.

On gimplification, lowering stuff and further, I think it's okay if targeting GOMP_target, but on my view, this is going to prevent us from emiting OpenCL or do some extended analysis of the code for error analysis to be offloaded. May be introduce some switch that turns on OpenCL generation or GOMP_targeting dependently on the state?

I hope my colleagues will correct me, if I forgot something.

-
Thanks,  
    Evgeny.