From: Bernd Schmidt <bschmidt@redhat.com>
To: Thomas Schwinge <thomas@codesourcery.com>,
Jakub Jelinek <jakub@redhat.com>
Cc: gcc@gcc.gnu.org, gcc-patches@gcc.gnu.org, Jeff Law <law@redhat.com>
Subject: Re: OpenACC (gomp-4_0-branch) patch review
Date: Fri, 16 Oct 2015 10:11:00 -0000 [thread overview]
Message-ID: <5620CCC0.4010507@redhat.com> (raw)
In-Reply-To: <87lhb3b11q.fsf@kepler.schwinge.homeip.net>
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
next prev parent reply other threads:[~2015-10-16 10:11 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-10-13 19:12 Merge from gomp-4_1-branch to trunk Jakub Jelinek
2015-10-14 7:34 ` Sebastian Huber
2015-10-14 8:04 ` Jakub Jelinek
2015-10-14 8:10 ` Sebastian Huber
2015-10-14 8:32 ` Sebastian Huber
2015-10-16 9:47 ` OpenACC (gomp-4_0-branch) patch review (was: Merge from gomp-4_1-branch to trunk) Thomas Schwinge
2015-10-16 10:11 ` Bernd Schmidt [this message]
2015-10-16 10:15 ` Jakub Jelinek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5620CCC0.4010507@redhat.com \
--to=bschmidt@redhat.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=gcc@gcc.gnu.org \
--cc=jakub@redhat.com \
--cc=law@redhat.com \
--cc=thomas@codesourcery.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).