OpenACC (gomp-4_0-branch) patch review

[Bernd Schmidt]

On 10/16/2015 11:44 AM, Thomas Schwinge wrote:
> Lately, Bernd has
> stepped up a few times to review OMP patches (many thanks, Bernd!), but
> also he sometimes stated that even though a patch appears fine to him,
> he'd like "Jakub to have a look".

I'll just comment on this briefly. In general I'll try to review 
anything I think I can figure out and which doesn't have a super-active 
maintainer, but some areas have folks who clearly know the code better. 
The whole gomp area is one of those, and when in doubt, I like to err on 
the side of allowing (in this case) Jakub a chance to take a look.

> There's still a bit of clean-up and development going on on
> gomp-4_0-branch, but what should be the strategy to get it merged into
> trunk?  Instead of us extracting and submitting individual changes (which
> we certainly can do, but which is a huge time-sink if they're then not
> handled quickly), would you maybe prefer to do a "whole-branch" review?

It might be good to start with a relatively high-level overview of the 
current approach, also documenting which parts of OpenACC the changes 
implement and which ones they don't.

> would it perhaps make sense to officially
> appoint somebody else to review OMP changes in addition to you?  And,
> also, allow for "somewhat non-perfect" changes to be committed, and later
> address the "warts"?

Depends on how "somewhat non-perfect" is defined - you might want to 
elaborate. Code should follow the coding standards, add documentation, 
testcases, etc., those are minimum requirements for everyone. In cases 
where something is implemented in a way that has a clearly superiour 
alternative, I think it is reasonable to ask for it to be changed (the 
builtin folding fell into this category for me). In terms of the current 
general approach towards implementing OpenACC I don't intend to give you 
a hard time, especially since past patch review from Jakub pointed in 
this direction and it would be unreasonable to second-guess this choice now.

On the bright side I think most of omp-low.c could be described as 
"somewhat non-perfect", so you'd be following existing practice.


Bernd