This is the mail archive of the
mailing list for the GCC project.
Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to test
- From: Bernd Schmidt <bschmidt at redhat dot com>
- To: Thomas Schwinge <thomas at codesourcery dot com>, gcc-patches at gcc dot gnu dot org
- Cc: Jakub Jelinek <jakub at redhat dot com>, gcc at gcc dot gnu dot org, David Edelsohn <edelsohn at gnu dot org>
- Date: Fri, 5 Aug 2016 17:26:27 +0200
- Subject: Re: [GCC Steering Committee attention] [PING] [PING] [PING] libgomp: In OpenACC testing, cycle though $offload_targets, and by default only build for the offload target that we're actually going to test
- Authentication-results: sourceware.org; auth=none
- References: <firstname.lastname@example.org> <email@example.com> <firstname.lastname@example.org> <email@example.com>
On 08/04/2016 04:49 PM, Thomas Schwinge wrote:
Global Reviewers are welcome to review OpenACC/OpenMP/offloading patches.
But that doesn't help if that's then not happening in reality. (With the
exception of Bernd, who then did review such patches for a while, but
also seems to have stopped with that again.)
As for this particular patch, I saw it go by, but there were a number of
things that stopped me from looking at it which I'll try to explain. The
one quoted seems to be just a resubmission of one that was posted
towards the end of this thread:
(The resubmission should have contained a pointer to that thread).
That's a submission of three patches. If you follow that thread, you'll
see that Jakub objected to one of them, asking you to provide a smaller
one with just the necessary functional changes. In response you posted
what appears to be just a merge of all three patches together. From my
point of view, I see no reason to go near it if Jakub has unaddressed
It's possible that I've misunderstood something, but that leads me to
one piece of advice I could give (and I'm repeating myself here): write
clearer patch introductions. After reading this:
On Wed, 13 Jul 2016 12:37:07 +0200, I wrote:
As discussed before, "offloading compilation is slow; I suppose because
of having to invoke several tools (LTO streaming -> mkoffload -> offload
compilers, assemblers, linkers -> combine the resulting images; but I
have not done a detailed analysis on that)". For this reason it is
beneficial (that is, it is measurable in libgomp testing wall time) to
limit offload compilation to the one (in the OpenACC case) offload target
that we're actually going to test (that is, execute). Another reason is
that -foffload=-fdump-tree-[...] produces clashes (that is,
unpredicatable outcome) in the file names of offload compilations' dump
files' names. Here is a patch to implement that, to specify
-foffload=[...] during libgomp OpenACC testing. As that has been
| [...] there actually is a difference between offload_plugins and
| offload_targets (for example, "intelmic"
| vs. "x86_64-intelmicemul-linux-gnu"), and I'm using both variables --
| to avoid having to translate the more specific
| "x86_64-intelmicemul-linux-gnu" (which we required in the test harness)
| into the less specific "intelmic" (for plugin loading) in
| libgomp/target.c. I can do that, so that we can continue to use just a
| single offload_targets variable, but I consider that a less elegant
I'm already somewhat confused about what you want to achieve and how. It
sounds like you're trying to fix a dejagnu issue but then I look at the
patch and see lots of libgomp configury changes.
Avoid writing information that is irrelevant to the patch at hand, such
as most of the first sentence in the block quoted above. Don't quote
yourself from past discussions, write something new that's specific and
relevant to someone who's going to try and understand your patch. Try
not to have three or four parenthetical comments in one sentence.
A better way to write the first paragraph might be:
"When testing libgomp for a compiler that supports more than one offload
target, we iterate over the targets and run the test for each. That
currently produces binaries for all offload targets, each time, which is
wasted effort. Hence, this patch, which sets -foffload=xyz as
appropriate. This speeds up the compiler and also avoids name clashes
between dump files."
That even adds information that was missing from your submission: I had
to look at the patch to see that we iterate over targets, and that's
relevant to the question of whether we need the patch or not.
Since the patch does way more, that needs to be adequately explained,
and your original idea of a multi-part submission was in fact correct.
You need to somehow deal with the fact that one of them was rejected,
either using persuasion or reworking the other patches (possibly showing
that such a reworking would be significantly worse).