Re: Splitting up gcc/omp-low.c?

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 4852 bytes --]

Hi!

On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Wed, Dec 09, 2015 at 06:23:22PM +0100, Bernd Schmidt wrote:
> > > On 12/09/2015 05:24 PM, Thomas Schwinge wrote:
> > > >how about we split up gcc/omp-low.c into several
> > > >files?  Would it make sense (I have not yet looked in detail) to do so
> > > >along the borders of the several passes defined therein?

> > > I suspect a split along the ompexp/omplow boundary would be quite easy to
> > > achieve.

That was indeed the first one that I tackled, omp-expand.c (spelled out
"expand" instead of "exp" to avoid confusion as "exp" might also be short
for "expression"; OK?), and a omp-offload.c also fell out of that (with
more content to be moved into there, I suspect).

We could split up omp-offload.c even further, but I don't know if that's
really feasible.  Currently in there: offload tables stuff, OpenACC loops
stuff and pass_oacc_device_lower, pass_omp_target_link; separated by ^L
in this one file.

> And possibly some kind of omp-simd.c, and omp-checking.c, and so
> on, if feasible.  (I have not yet looked in detail.)

Not yet looked into these.

Stuff that does not relate to OMP lowering, I did not move stuff out of
omp-low.c (into a new omp.c, or omp-misc.c, for example) so far, but
instead just left all that in omp-low.c.  We'll see how far we get.

One thing I noticed is that there sometimes is more than one suitable
place to put stuff: omp-low.c and omp-expand.c categorize by compiler
passes, and omp-offload.c -- at least in part -- is about the orthogonal
"offloading" category.  For example, see the OMPTODO "struct oacc_loop
and enum oacc_loop_flags" in gcc/omp-offload.h.  We'll see how that goes.

> > > >I'd suggest to do this shortly before GCC 6 is released, [...]

Here is a first variant of such a patch.  I will continue to maintain
this, and intend to send (incremental?) patches on top of that one, but
intend to eventually commit all changes as one big commit, to avoid too
much "source code churn" (as much as that's possible...).

Some more comments, to help review:

The attached 0001-Split-up-gcc-omp-low.c.patch.xz is a Git "--color
--word-diff --ignore-space-change" patch, purely meant for manual review;
I'm intentionally ;-) not attaching a "patch-applyable" patch at this
point, to minimize the risk of other people starting to work on this in
parallel with my ongoing changes, which no doubt would result in a ton of
patch merge conflicts.  Yet, I'm of course very open to any kind of
suggestions; please submit these as a "verbal patch".  I will of course
submit a patch in any other format that you'd like for review.

This already survived "light" C/C++/Fortran
--enable-offload-targets=nvptx-none,x86_64-intelmicemul-linux-gnu,hsa
testing (no HSA execution testing, though), and also survived a "big"
bootstrap build.

As I don't know how this is usually done: is it appropriate to remove
"Contributed by Diego Novillo" from omp-low.c (he does get mentioned for
his OpenMP work in gcc/doc/contrib.texi; a ton of other people have been
contributing a ton of other stuff since omp-low.c has been created), or
does this line stay in omp-low.c, or do I even duplicate it into the new
files?

I tried not to re-order stuff when moving.  But: we may actually want to
reorder stuff, to put it into a more sensible order.  Any suggestions?

All lines with "//OMP" tags in them will eventually be removed; these are
just to help review (hence the --word-diff patch), and to solicit
comments, in the case of "//OMPTODO".  Some of the OMPTODOs are for
myself (clean up #include directives), but for the others, I'd like to
hear any comments that you have.

I guess you can just ignore any "//OMPCUT" tags (and I'll remove them at
one point, and clean up the whitespace).  (In the new files) these mean
that in the file where the surrounding stuff is from, there has been
other stuff that either remained in the original file (omp-low.c), or has
been moved to other files.

In omp-low.c and omp-low.h, a "//OMPLOWH" tag means that this line has
been moved to omp-low.h, and likewise: "//OMPEXP" to omp-expand.c,
"//OMPEXPH" to omp-expand.h, "//OMPOFF" to omp-offload.c, and "//OMPOFFH"
to omp-offload.h.

I had to export a small number of functions (see the prototypes not moved
but added to the header files).

Because it's also used in omp-expand.c, I moved the one-line static
inline is_reference function from omp-low.c to omp-low.h, and renamed it
to omp_is_reference because of the very generic name.  Similar functions
stay in omp-low.c however, so they're no longer defined next to each
other.  OK, or does this need a different solution?


Grüße
 Thomas



[-- Attachment #2: 0001-Split-up-gcc-omp-low.c.patch.xz --]
[-- Type: application/x-xz, Size: 94664 bytes --]