This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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:

<https://gcc.gnu.org/ml/gcc-patches/2016-04/msg01787.html>.

(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 objections.

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
challenged before:

| [...] 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
| solution.

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).


Bernd


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]