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

[Martin Jambor]

Hi,

On Wed, May 25, 2016 at 08:03:41AM +0200, Thomas Schwinge wrote:
> Hi!
> 
> Ping.
> 
> Given that we conceptually agreed about this task, but apparently nobody
> is now interested in reviewing my proposed changes (and tells me how
> they'd like me to submit the patch for review), should I maybe just
> execute the steps?

I would suggest that you re-post the patch in a new thread (and in an
orinary non-fancy format), the details are now buried in this big
thred and might easily be forgotten.

I would also strongly suggest that you post the Changelog in the email
body, even if you compress and attach the patch itself.  Frankly, if
the changes are just mechanical movements, only the Changelog is what
I'd like to see and perhaps comment on.

Martin

> 
> On Wed, 18 May 2016 13:42:37 +0200, Thomas Schwinge  
> wrote:
> > Ping.
> > 
> > On Wed, 11 May 2016 15:44:14 +0200, I wrote:
> > > Ping.
> > > 
> > > On Tue, 03 May 2016 11:34:39 +0200, I wrote:
> > > > On Wed, 13 Apr 2016 18:01:09 +0200, I wrote:
> > > > > On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> > > > > > On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek 
> > > > > >  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?) [...]
> > > > 
> > > > That's the one I'd suggest to pursue next, now that GCC 6.1 has been
> > > > released.  How would you like me to submit the patch for review?  (It's
> > > > huge, obviously.)
> > > > 
> > > > A few high-level comments, and questions that remain to be answered:
> > > > 
> > > > > 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 -- [would be] 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.
> > > > 
> > > > > Some more comments, to help review:
> > > > 
> > > > > 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?
> > > > 
> > > > > 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

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

[Thomas Schwinge]

Hi!

Ping.

Given that we conceptually agreed about this task, but apparently nobody
is now interested in reviewing my proposed changes (and tells me how
they'd like me to submit the patch for review), should I maybe just
execute the steps?

On Wed, 18 May 2016 13:42:37 +0200, Thomas Schwinge  
wrote:
> Ping.
> 
> On Wed, 11 May 2016 15:44:14 +0200, I wrote:
> > Ping.
> > 
> > On Tue, 03 May 2016 11:34:39 +0200, I wrote:
> > > On Wed, 13 Apr 2016 18:01:09 +0200, I wrote:
> > > > On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> > > > > On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  
> > > > > 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?) [...]
> > > 
> > > That's the one I'd suggest to pursue next, now that GCC 6.1 has been
> > > released.  How would you like me to submit the patch for review?  (It's
> > > huge, obviously.)
> > > 
> > > A few high-level comments, and questions that remain to be answered:
> > > 
> > > > 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 -- [would be] 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.
> > > 
> > > > Some more comments, to help review:
> > > 
> > > > 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?
> > > 
> > > > 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

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

[Thomas Schwinge]

Hi!

Ping.

On Wed, 11 May 2016 15:44:14 +0200, I wrote:
> Ping.
> 
> On Tue, 03 May 2016 11:34:39 +0200, I wrote:
> > On Wed, 13 Apr 2016 18:01:09 +0200, I wrote:
> > > On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> > > > On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  
> > > > 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?) [...]
> > 
> > That's the one I'd suggest to pursue next, now that GCC 6.1 has been
> > released.  How would you like me to submit the patch for review?  (It's
> > huge, obviously.)
> > 
> > A few high-level comments, and questions that remain to be answered:
> > 
> > > 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 -- [would be] 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.
> > 
> > > Some more comments, to help review:
> > 
> > > 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?
> > 
> > > 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

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

[Thomas Schwinge]

Hi!

Ping.

On Tue, 03 May 2016 11:34:39 +0200, I wrote:
> On Wed, 13 Apr 2016 18:01:09 +0200, I wrote:
> > On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> > > On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  
> > > 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?) [...]
> 
> That's the one I'd suggest to pursue next, now that GCC 6.1 has been
> released.  How would you like me to submit the patch for review?  (It's
> huge, obviously.)
> 
> A few high-level comments, and questions that remain to be answered:
> 
> > 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 -- [would be] 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.
> 
> > Some more comments, to help review:
> 
> > 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?
> 
> > 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

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

[Thomas Schwinge]

Hi!

On Wed, 13 Apr 2016 18:01:09 +0200, I wrote:
> On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> > On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  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?) [...]

That's the one I'd suggest to pursue next, now that GCC 6.1 has been
released.  How would you like me to submit the patch for review?  (It's
huge, obviously.)

A few high-level comments, and questions that remain to be answered:

> 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 -- [would be] 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.

> Some more comments, to help review:

> 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?

> 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

Re: Split out OMP constructs' SIMD clone supporting code (was: Splitting up gcc/omp-low.c?)

[Thomas Schwinge]

Hi!

On Fri, 15 Apr 2016 14:15:42 +0200, Jakub Jelinek  wrote:
> On Fri, Apr 15, 2016 at 02:11:45PM +0200, Thomas Schwinge wrote:
> > On Fri, 15 Apr 2016 13:57:05 +0200, Jakub Jelinek  wrote:
> > > On Fri, Apr 15, 2016 at 01:53:14PM +0200, Thomas Schwinge wrote:
> > > > For all the other splitting patches that I have posted/proposed, the 
> > > > idea
> > > > then is to commit these onto both gcc-6-branch and trunk?
> > > 
> > > If we branch today, then yes, though the gcc-6-branch commits would need 
> > > to
> > > wait until after 6.1 is released.
> > 
> > Uh.  I fear these patches will be a bit of a pain to maintain if there's
> > then on-going development in trunk, touching the gcc/omp-low.c file?  I
> > thought we had agreed to get these cleanup changes into trunk in time for
> > the first GCC 6 release, to exactly avoid this kind of pain (that is,
> > divergence of the gcc/omp-low.c files in trunk and gcc-6-branch)?  :-( I
> > have not been aware that you intend to create the gcc-6-branch today, and
> > neither have I been aware that once the branch's created, the other
> > cleanup changes would have to wait until after the GCC 6.1 release.
> > There is no way I can get all this done today, or on the weekend.
> 
> Then postpone the rest shortly before 6.1 is released.
> We shouldn't have big changes to trunk before 6.1 is released anyway,
> otherwise we won't be able to test on the trunk fixes intended also for 6.1.

Hmm.  What about the further patches that I already have in the queue for
submission, just waiting for final testing/polishing?  Are you asking me
to rescind these now, and then have to reproduce the work later on (when
exactly?), or will it still be fine to get these approved and committed
in the near future (next days)?  Obviously, I'll strongly prefer the
latter, as I already have done most of the work, and as that's what I
understand to be the modus operandi we had agreed on.


Grüße
 Thomas

Re: Split out OMP constructs' SIMD clone supporting code (was: Splitting up gcc/omp-low.c?)

[Jakub Jelinek]

On Fri, Apr 15, 2016 at 02:11:45PM +0200, Thomas Schwinge wrote:
> Hi!
> 
> On Fri, 15 Apr 2016 13:57:05 +0200, Jakub Jelinek  wrote:
> > On Fri, Apr 15, 2016 at 01:53:14PM +0200, Thomas Schwinge wrote:
> > > For all the other splitting patches that I have posted/proposed, the idea
> > > then is to commit these onto both gcc-6-branch and trunk?
> > 
> > If we branch today, then yes, though the gcc-6-branch commits would need to
> > wait until after 6.1 is released.
> 
> Uh.  I fear these patches will be a bit of a pain to maintain if there's
> then on-going development in trunk, touching the gcc/omp-low.c file?  I
> thought we had agreed to get these cleanup changes into trunk in time for
> the first GCC 6 release, to exactly avoid this kind of pain (that is,
> divergence of the gcc/omp-low.c files in trunk and gcc-6-branch)?  :-( I
> have not been aware that you intend to create the gcc-6-branch today, and
> neither have I been aware that once the branch's created, the other
> cleanup changes would have to wait until after the GCC 6.1 release.
> There is no way I can get all this done today, or on the weekend.

Then postpone the rest shortly before 6.1 is released.
We shouldn't have big changes to trunk before 6.1 is released anyway,
otherwise we won't be able to test on the trunk fixes intended also for 6.1.

Jakub

Re: Split out OMP constructs' SIMD clone supporting code (was: Splitting up gcc/omp-low.c?)

[Thomas Schwinge]

Hi!

On Fri, 15 Apr 2016 13:57:05 +0200, Jakub Jelinek  wrote:
> On Fri, Apr 15, 2016 at 01:53:14PM +0200, Thomas Schwinge wrote:
> > For all the other splitting patches that I have posted/proposed, the idea
> > then is to commit these onto both gcc-6-branch and trunk?
> 
> If we branch today, then yes, though the gcc-6-branch commits would need to
> wait until after 6.1 is released.

Uh.  I fear these patches will be a bit of a pain to maintain if there's
then on-going development in trunk, touching the gcc/omp-low.c file?  I
thought we had agreed to get these cleanup changes into trunk in time for
the first GCC 6 release, to exactly avoid this kind of pain (that is,
divergence of the gcc/omp-low.c files in trunk and gcc-6-branch)?  :-( I
have not been aware that you intend to create the gcc-6-branch today, and
neither have I been aware that once the branch's created, the other
cleanup changes would have to wait until after the GCC 6.1 release.
There is no way I can get all this done today, or on the weekend.


Grüße
 Thomas

Re: Split out OMP constructs' SIMD clone supporting code (was: Splitting up gcc/omp-low.c?)

[Jakub Jelinek]

On Fri, Apr 15, 2016 at 01:53:14PM +0200, Thomas Schwinge wrote:
> Hi!
> 
> On Fri, 15 Apr 2016 13:15:07 +0200, Jakub Jelinek  wrote:
> > On Thu, Apr 14, 2016 at 10:27:40PM +0200, Thomas Schwinge wrote:
> > > On Thu, 14 Apr 2016 18:01:13 +0200, I wrote:
> > > > "simdclone" pass, with the
> > > > respective supporting code.  I will certainly submit line-diff patches 
> > > > if
> > > > we agree that this is sound -- these two may actually be good candidates
> > > > to do first, individually, and do that now, because they're completely
> > > > self-contained.  Makes sense?
> 
> > > OK to commit once bootstrap testing succeeded?
> > 
> > Ok
> 
> Committed without changes in r235017.
> 
> > if you manage to do so before the (hopefully intermittent) branching.
> 
> For all the other splitting patches that I have posted/proposed, the idea
> then is to commit these onto both gcc-6-branch and trunk?

If we branch today, then yes, though the gcc-6-branch commits would need to
wait until after 6.1 is released.

Jakub

Re: Split out OMP constructs' SIMD clone supporting code (was: Splitting up gcc/omp-low.c?)

[Thomas Schwinge]

Hi!

On Fri, 15 Apr 2016 13:15:07 +0200, Jakub Jelinek  wrote:
> On Thu, Apr 14, 2016 at 10:27:40PM +0200, Thomas Schwinge wrote:
> > On Thu, 14 Apr 2016 18:01:13 +0200, I wrote:
> > > "simdclone" pass, with the
> > > respective supporting code.  I will certainly submit line-diff patches if
> > > we agree that this is sound -- these two may actually be good candidates
> > > to do first, individually, and do that now, because they're completely
> > > self-contained.  Makes sense?

> > OK to commit once bootstrap testing succeeded?
> 
> Ok

Committed without changes in r235017.

> if you manage to do so before the (hopefully intermittent) branching.

For all the other splitting patches that I have posted/proposed, the idea
then is to commit these onto both gcc-6-branch and trunk?


Grüße
 Thomas

Re: Split out OMP constructs' SIMD clone supporting code (was: Splitting up gcc/omp-low.c?)

[Jakub Jelinek]

On Thu, Apr 14, 2016 at 10:27:40PM +0200, Thomas Schwinge wrote:
> On Thu, 14 Apr 2016 18:01:13 +0200, I wrote:
> > "simdclone" pass, with the
> > respective supporting code.  I will certainly submit line-diff patches if
> > we agree that this is sound -- these two may actually be good candidates
> > to do first, individually, and do that now, because they're completely
> > self-contained.  Makes sense?
> 
> ;-) Made enough sense to me, so that I prepared the attached patch.  I'm
> also attaching a "-C" variant that I created using Git's -C5% option, and
> which shows how the new file gcc/omp-simd-clone.c can be created from the
> original gcc/omp-low.c.  (Is that useful for review, or are you manually
> doing something like that anyway?)
> 
> > Should possibly rename omp-simd.c to omp-simd-clone.c to make it clear
> > that's the only thing it does, the "simdclone" pass?
> 
> I did that.
> 
> I manually determined a reduced #include list for the new file
> gcc/omp-simd-clone.c.  Hope that's alright.
> 
> OK to commit once bootstrap testing succeeded?

Ok if you manage to do so before the (hopefully intermittent) branching.

> commit 8f33dc59ad24a995694d42ee9013e0853426e190
> Author: Thomas Schwinge 
> Date:   Thu Apr 14 21:56:31 2016 +0200
> 
> Split out OMP constructs' SIMD clone supporting code
> 
>   gcc/
>   * omp-low.c (simd_clone_struct_alloc, simd_clone_struct_copy)
>   (simd_clone_vector_of_formal_parm_types)
>   (simd_clone_clauses_extract, simd_clone_compute_base_data_type)
>   (simd_clone_mangle, simd_clone_create)
>   (simd_clone_adjust_return_type, create_tmp_simd_array)
>   (simd_clone_adjust_argument_types, simd_clone_init_simd_arrays)
>   (struct modify_stmt_info, ipa_simd_modify_stmt_ops)
>   (ipa_simd_modify_function_body, simd_clone_linear_addend)
>   (simd_clone_adjust, expand_simd_clones, ipa_omp_simd_clone)
>   (pass_data_omp_simd_clone, class pass_omp_simd_clone)
>   (pass_omp_simd_clone::gate, make_pass_omp_simd_clone): Move
>   into...
>   * omp-simd-clone.c: ... this new file.
>   (simd_clone_vector_of_formal_parm_types): Make it static.
>   * Makefile.in (OBJS): Add omp-simd-clone.o.
> 
>  gcc/Makefile.in  |1 +
>  gcc/omp-low.c| 1606 
>  gcc/omp-simd-clone.c | 1654 
> ++
>  3 files changed, 1655 insertions(+), 1606 deletions(-)

Jakub

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

[Thomas Schwinge]

Hi!

On Wed, 13 Apr 2016 18:01:09 +0200, I wrote:
> On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> > On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  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.

> > And possibly some kind of omp-simd.c, and omp-checking.c, and so
> > on, if feasible.
> 
> Not yet looked into these.

..., and here they are: word-diff patches creating omp-diagnostics.c and
omp-simd.c, 0001-new-file-gcc-omp-diagnostics.c.patch.xz and
0002-new-file-gcc-omp-simd.c.patch.xz.  The former contains the
"diagnose_omp_blocks" pass and the latter the "simdclone" pass, with the
respective supporting code.  I will certainly submit line-diff patches if
we agree that this is sound -- these two may actually be good candidates
to do first, individually, and do that now, because they're completely
self-contained.  Makes sense?

Should possibly rename omp-simd.c to omp-simd-clone.c to make it clear
that's the only thing it does, the "simdclone" pass?

Should we maybe rename omp-diagnostics.c to omp-structured-blocks.c, and
really only have it contain that "diagnose_omp_blocks" pass, or should we
move other diagnostic stuff like check_omp_nesting_restrictions into that
file, too?

One //OMPTODO:

--- gcc/omp-low.h
+++ gcc/omp-low.h
@@ -95,0 +96,11 @@ extern gimple *build_omp_barrier (tree);
+//OMPTODO: moved from omp-low.c.  Renamed from WALK_SUBSTMTS to 
OMP_WALK_SUBSTMTS because of the very generic name.  Used in omp-low.c and 
omp-diagnostics.c.  Alternatively, WALK_SUBSTMTS should perhaps simply be 
duplicated in the two files?
+#define OMP_WALK_SUBSTMTS  \
+case GIMPLE_BIND: \
+case GIMPLE_TRY: \
+case GIMPLE_CATCH: \
+case GIMPLE_EH_FILTER: \
+case GIMPLE_TRANSACTION: \
+  /* The sub-statements for these should be walked.  */ \
+  *handled_ops_p = false; \
+  break;

Instead of that, duplicate WALK_SUBSTMTS into both omp-low.c and
omp-diagnostics.c?


Grüße
 Thomas




0001-new-file-gcc-omp-diagnostics.c.patch.xz
Description: application/xz


0002-new-file-gcc-omp-simd.c.patch.xz
Description: application/xz

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

[Bernd Schmidt]

On 04/14/2016 03:36 PM, Thomas Schwinge wrote:


I don't know how meaningful it is to create/discuss/review/commit the
following patch until the overall approach has been agreed upon?


Why not? We agree the file should be split, and this makes it easier. So 
I'll approve it for stage1, or earlier if RMs want this split in gcc-6.


You could however probably also remove the #include langhooks from the C 
file.



Bernd

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

[Thomas Schwinge]

Hi!

On Wed, 13 Apr 2016 20:20:19 +0200, Bernd Schmidt  wrote:
> On 04/13/2016 07:56 PM, Thomas Schwinge wrote:
> > 0001-Split-up-gcc-omp-low.c-plain.patch.xz attached.
> 
> That looks much better. However, the //OMPWHATEVER comments are not 
> really all that helpful.

They're helpful in the word-diff patch, as they show where I moved stuff.
I agree that they can't serve this purpose in the line-diff patch.

> I think the next step would be to clear out all 
> this stuff including the OMPCUT markers

I think the first step would be to get agreement on the several questions
that I posted yesterday.  That is, which stuff to move, into which files,
how to structure it all, and so on.  That's what I hoped the word-diff
patch would help with.  Everything else sounds a bit like busywork to me;
that is:

> start with an 
> initial patch that includes everything that actually modifies code 
> rather than moving it (omp_is_reference seems to be the major thing here).

I don't know how meaningful it is to create/discuss/review/commit the
following patch until the overall approach has been agreed upon?

commit 8ac6b83350022c74bb2af8006e51f3620889e7c1
Author: Thomas Schwinge 
Date:   Thu Apr 14 15:25:24 2016 +0200

gcc/omp-low.c:is_reference -> gcc/omp-low.h:omp_is_reference

gcc/
* omp-low.h: Include "langhooks.h".  Define omp_is_reference here,
and...
* omp-low.c: ... don't define is_reference here.  Adjust all
users.
---
 gcc/omp-low.c | 102 +++---
 gcc/omp-low.h |  10 ++
 2 files changed, 57 insertions(+), 55 deletions(-)

diff --git gcc/omp-low.c gcc/omp-low.c
index 7335abc..bb9c61e 100644
--- gcc/omp-low.c
+++ gcc/omp-low.c
@@ -1039,21 +1039,13 @@ is_variable_sized (const_tree expr)
   return !TREE_CONSTANT (TYPE_SIZE_UNIT (TREE_TYPE (expr)));
 }
 
-/* Return true if DECL is a reference type.  */
-
-static inline bool
-is_reference (tree decl)
-{
-  return lang_hooks.decls.omp_privatize_by_reference (decl);
-}
-
 /* Return the type of a decl.  If the decl is reference type,
return its base type.  */
 static inline tree
 get_base_type (tree decl)
 {
   tree type = TREE_TYPE (decl);
-  if (is_reference (decl))
+  if (omp_is_reference (decl))
 type = TREE_TYPE (type);
   return type;
 }
@@ -1348,7 +1340,7 @@ build_outer_var_ref (tree var, omp_context *ctx, bool 
lastprivate = false)
}
x = lookup_decl (var, outer);
 }
-  else if (is_reference (var))
+  else if (omp_is_reference (var))
 /* This can happen with orphaned constructs.  If var is reference, it is
possible it is shared and as such valid.  */
 x = var;
@@ -1371,7 +1363,7 @@ build_outer_var_ref (tree var, omp_context *ctx, bool 
lastprivate = false)
}
 }
 
-  if (is_reference (var))
+  if (omp_is_reference (var))
 x = build_simple_mem_ref (x);
 
   return x;
@@ -1433,7 +1425,7 @@ install_var_field (tree var, bool by_ref, int mask, 
omp_context *ctx,
   if (base_pointers_restrict)
type = build_qualified_type (type, TYPE_QUAL_RESTRICT);
 }
-  else if ((mask & 3) == 1 && is_reference (var))
+  else if ((mask & 3) == 1 && omp_is_reference (var))
 type = TREE_TYPE (type);
 
   field = build_decl (DECL_SOURCE_LOCATION (var),
@@ -1904,7 +1896,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx,
  if ((! TREE_READONLY (decl) && !OMP_CLAUSE_SHARED_READONLY (c))
  || TREE_ADDRESSABLE (decl)
  || by_ref
- || is_reference (decl))
+ || omp_is_reference (decl))
{
  by_ref = use_pointer_for_field (decl, ctx);
  install_var_field (decl, by_ref, 3, ctx);
@@ -1954,7 +1946,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx,
  && is_gimple_omp_offloaded (ctx->stmt))
{
  if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_FIRSTPRIVATE)
-   install_var_field (decl, !is_reference (decl), 3, ctx);
+   install_var_field (decl, !omp_is_reference (decl), 3, ctx);
  else if (TREE_CODE (TREE_TYPE (decl)) == ARRAY_TYPE)
install_var_field (decl, true, 3, ctx);
  else
@@ -1973,7 +1965,7 @@ scan_sharing_clauses (tree clauses, omp_context *ctx,
  by_ref = use_pointer_for_field (decl, NULL);
 
  if (is_task_ctx (ctx)
- && (global || by_ref || is_reference (decl)))
+ && (global || by_ref || omp_is_reference (decl)))
{
  install_var_field (decl, false, 1, ctx);
  if (!global)
@@ -4673,10 +4665,10 @@ lower_rec_input_clauses (tree clauses, gimple_seq 
*ilist, gimple_seq *dlist,
  tree ref = build_outer_var_ref (var, ctx);
  /* For ref build_outer_var_ref already performs this.  */
  if (TREE_CODE (d) == INDIRECT_REF)
-   

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

[Bernd Schmidt]

On 04/13/2016 07:56 PM, Thomas Schwinge wrote:


Best way to present this might be to do
diff -du old-omp-low.c .


OK, I found Git "-C5%" produce something very similar to that;
0001-Split-up-gcc-omp-low.c-plain.patch.xz attached.


That looks much better. However, the //OMPWHATEVER comments are not 
really all that helpful. I think the next step would be to clear out all 
this stuff including the OMPCUT markers, and also to start with an 
initial patch that includes everything that actually modifies code 
rather than moving it (omp_is_reference seems to be the major thing here).



Bernd

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

[Bernd Schmidt]

On 04/13/2016 06:01 PM, Thomas Schwinge wrote:


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.


I have no idea how to read this patch. I can't even properly show it 
with "less" because it seems to contain color escape sequences. The 
word-diff format (I assume that's what it is) is also unfamiliar and not 
immediately readable.


Best way to present this might be to do
diff -du old-omp-low.c .


Bernd

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

[Thomas Schwinge]

Hi!

On Fri, 08 Apr 2016 11:36:03 +0200, I wrote:
> On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  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




0001-Split-up-gcc-omp-low.c.patch.xz
Description: application/xz

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

[Jakub Jelinek]

On Fri, Apr 08, 2016 at 11:36:03AM +0200, Thomas Schwinge wrote:
> > Certainly.  On one side I'd say it is too late now in stage3, on the other
> > side when would be better time to do that, during stage1 people will have
> > more likely out of the tree branches with more changes (I'm aware we even
> > now have the HSA, OpenMP -> PTX and OpenACC branches).
> > 
> > So, if somebody wants to try that, we can see if the result would be
> > appropriate.
> 
> So, has time now come to execute this task?  (To remind: the idea
> explicitly has been to do this late, shortly before the gcc-6-branch gets
> created, to make it easy in the following months to apply patches to both
> trunk and gcc-6-branch.)

Only if you are able to do it quickly, branching is approaching very fast,
we have just last couple of P1s and are already below 100 P1-P3s.
I hope we can branch next week and release in 2 weeks.

Jakub

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

[Martin Jambor]

Hi,

On Fri, Apr 08, 2016 at 11:36:03AM +0200, Thomas Schwinge wrote:
> Hi!
> 
> On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  wrote:
> > On Wed, Dec 09, 2015 at 06:23:22PM +0100, Bernd Schmidt wrote:
> > > On 12/09/2015 05:24 PM, Thomas Schwinge wrote:
> > > >
> > > >In addition to that, 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?  Or, can you
> > > >tell already that there would be too many cross-references between the
> > > >several files to make this infeasible?
> > > 
> > > It would be nice to get rid of all the code duplication in that file. That
> > > alone could reduce the size by quite a bit, and hopefully make it easier 
> > > to
> > > read.
> > 
> > What exact code duplication do you mean?
> 
> (Has been discussed in the following.)  At this point, I do not intend to
> work on any kinds of cleanup, but rather just the "mechanical" changes:
> 
> > > I suspect a split along the ompexp/omplow boundary would be quite easy to
> > > achieve.
> > 
> > Yeah, that might be the possible splitting boundary (have omp-low.c,
> > omp-exp.c).
> 
> Right.  And possibly some kind of omp-simd.c, and omp-checking.c, and so
> on, if feasible.  (I have not yet looked in detail.)
> 
> > > >I'd suggest to do this shortly before GCC 6 is released, so that
> > > >backports from trunk to gcc-6-branch will be easy.  (I assume we don't
> > > >have to care for gcc-5-branch backports too much any longer.)
> > > 
> > > I'll declare myself agnostic as to whether such a change is appropriate 
> > > for
> > > gcc-6 at this stage. I guess it kind of depends on the specifics.
> > 
> > Certainly.  On one side I'd say it is too late now in stage3, on the other
> > side when would be better time to do that, during stage1 people will have
> > more likely out of the tree branches with more changes (I'm aware we even
> > now have the HSA, OpenMP -> PTX and OpenACC branches).
> > 
> > So, if somebody wants to try that, we can see if the result would be
> > appropriate.
> 
> So, has time now come to execute this task?  (To remind: the idea
> explicitly has been to do this late, shortly before the gcc-6-branch gets
> created, to make it easy in the following months to apply patches to both
> trunk and gcc-6-branch.)
> 

Unless someone is quicler, I can give it a go next Thursday (not any
sooner, unfortunately).  I would do a division into omp-low.c and
omp-exp.c and possibly an omp.c for simple stuff not fitting anywhere
else and perhaps even a separate omp-gridify.c.  Someone else would
have to put stuff into an omp-simd.c, I'm afraid.  But it we can go
about this incrementaly.

Thanks,

Martin

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

[Thomas Schwinge]

Hi!

On Thu, 10 Dec 2015 09:08:35 +0100, Jakub Jelinek  wrote:
> On Wed, Dec 09, 2015 at 06:23:22PM +0100, Bernd Schmidt wrote:
> > On 12/09/2015 05:24 PM, Thomas Schwinge wrote:
> > >
> > >In addition to that, 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?  Or, can you
> > >tell already that there would be too many cross-references between the
> > >several files to make this infeasible?
> > 
> > It would be nice to get rid of all the code duplication in that file. That
> > alone could reduce the size by quite a bit, and hopefully make it easier to
> > read.
> 
> What exact code duplication do you mean?

(Has been discussed in the following.)  At this point, I do not intend to
work on any kinds of cleanup, but rather just the "mechanical" changes:

> > I suspect a split along the ompexp/omplow boundary would be quite easy to
> > achieve.
> 
> Yeah, that might be the possible splitting boundary (have omp-low.c,
> omp-exp.c).

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

> > >I'd suggest to do this shortly before GCC 6 is released, so that
> > >backports from trunk to gcc-6-branch will be easy.  (I assume we don't
> > >have to care for gcc-5-branch backports too much any longer.)
> > 
> > I'll declare myself agnostic as to whether such a change is appropriate for
> > gcc-6 at this stage. I guess it kind of depends on the specifics.
> 
> Certainly.  On one side I'd say it is too late now in stage3, on the other
> side when would be better time to do that, during stage1 people will have
> more likely out of the tree branches with more changes (I'm aware we even
> now have the HSA, OpenMP -> PTX and OpenACC branches).
> 
> So, if somebody wants to try that, we can see if the result would be
> appropriate.

So, has time now come to execute this task?  (To remind: the idea
explicitly has been to do this late, shortly before the gcc-6-branch gets
created, to make it easy in the following months to apply patches to both
trunk and gcc-6-branch.)


Grüße
 Thomas

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

[Nathan Sidwell]

On 12/10/15 06:34, Jakub Jelinek wrote:


I'm aware of some duplication in expand_omp_for_* functions, and some of the
obvious duplications were already moved to helper functions.  But in these
cases the number of differences is even significantly bigger too, so having
just one function that would handle all the different schedules would be far
less readable.  Perhaps we can add some small helpers to handle some little
pieces that repeat between the functions.


I agree.  For instance, earlier openacc's loop expansion piggybacked onto the 
the two omp loop expanders.  I found it much cleaner to create a separate 
openacc loop expander.  There's so much stuff to juggle in each case, that 
combining all the variants into one function can lead to cognitive overload.


nathan

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

[Jakub Jelinek]

On Wed, Dec 09, 2015 at 06:23:22PM +0100, Bernd Schmidt wrote:
> On 12/09/2015 05:24 PM, Thomas Schwinge wrote:
> >
> >In addition to that, 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?  Or, can you
> >tell already that there would be too many cross-references between the
> >several files to make this infeasible?
> 
> It would be nice to get rid of all the code duplication in that file. That
> alone could reduce the size by quite a bit, and hopefully make it easier to
> read.

What exact code duplication do you mean?

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

Yeah, that might be the possible splitting boundary (have omp-low.c,
omp-exp.c).

> >I'd suggest to do this shortly before GCC 6 is released, so that
> >backports from trunk to gcc-6-branch will be easy.  (I assume we don't
> >have to care for gcc-5-branch backports too much any longer.)
> 
> I'll declare myself agnostic as to whether such a change is appropriate for
> gcc-6 at this stage. I guess it kind of depends on the specifics.

Certainly.  On one side I'd say it is too late now in stage3, on the other
side when would be better time to do that, during stage1 people will have
more likely out of the tree branches with more changes (I'm aware we even
now have the HSA, OpenMP -> PTX and OpenACC branches).

So, if somebody wants to try that, we can see if the result would be
appropriate.

Jakub

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

[Jakub Jelinek]

On Thu, Dec 10, 2015 at 12:26:10PM +0100, Bernd Schmidt wrote:
> On 12/10/2015 09:08 AM, Jakub Jelinek wrote:
> >On Wed, Dec 09, 2015 at 06:23:22PM +0100, Bernd Schmidt wrote:
> >>On 12/09/2015 05:24 PM, Thomas Schwinge wrote:
> >>>
> >>>In addition to that, 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?  Or, can you
> >>>tell already that there would be too many cross-references between the
> >>>several files to make this infeasible?
> >>
> >>It would be nice to get rid of all the code duplication in that file. That
> >>alone could reduce the size by quite a bit, and hopefully make it easier to
> >>read.
> >
> >What exact code duplication do you mean?
> 
> Functions that are near-identical with slight differences, or which have
> large sections of identical code. scan_omp_task vs scan_omp_parallel, or the

Even these two (quite short) have huge number of differences, so I'm not
100% sure it would be more readable if we had just one scan_omp_taskreg that
handled both.

> various expand_omp_for functions are examples.

I'm aware of some duplication in expand_omp_for_* functions, and some of the
obvious duplications were already moved to helper functions.  But in these
cases the number of differences is even significantly bigger too, so having
just one function that would handle all the different schedules would be far
less readable.  Perhaps we can add some small helpers to handle some little
pieces that repeat between the functions.

Jakub

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

[Bernd Schmidt]

On 12/10/2015 09:08 AM, Jakub Jelinek wrote:

On Wed, Dec 09, 2015 at 06:23:22PM +0100, Bernd Schmidt wrote:

On 12/09/2015 05:24 PM, Thomas Schwinge wrote:


In addition to that, 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?  Or, can you
tell already that there would be too many cross-references between the
several files to make this infeasible?


It would be nice to get rid of all the code duplication in that file. That
alone could reduce the size by quite a bit, and hopefully make it easier to
read.


What exact code duplication do you mean?


Functions that are near-identical with slight differences, or which have 
large sections of identical code. scan_omp_task vs scan_omp_parallel, or 
the various expand_omp_for functions are examples.



Bernd

Splitting up gcc/omp-low.c? (was: [hsa 5/10] OpenMP lowering/expansion changes (gridification))

[Thomas Schwinge]

Hi!

I've been meaning to suggest this for some time already:

On Wed, 9 Dec 2015 14:19:30 +0100, Jakub Jelinek  wrote:
> As for omp-low.c changes, the file is already large enough that it would be
> nice if it is easy to find out what routines are for gridification purposes
> only, use some special prefix (grid_*, ompgrid_*, ...) for all such
> functions?

In addition to that, 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?  Or, can you
tell already that there would be too many cross-references between the
several files to make this infeasible?

I'd suggest to do this shortly before GCC 6 is released, so that
backports from trunk to gcc-6-branch will be easy.  (I assume we don't
have to care for gcc-5-branch backports too much any longer.)

$ wc -l gcc/*.c | sort -r -n | head
  879881 total
   25770 gcc/dwarf2out.c
   19834 gcc/omp-low.c
   14419 gcc/fold-const.c
   14357 gcc/combine.c
   14003 gcc/tree.c
   11622 gcc/expr.c
   11610 gcc/gimplify.c
   10417 gcc/tree-vrp.c
   10328 gcc/var-tracking.c


Grüße
 Thomas


signature.asc
Description: PGP signature

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

[Bernd Schmidt]

On 12/09/2015 05:24 PM, Thomas Schwinge wrote:


In addition to that, 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?  Or, can you
tell already that there would be too many cross-references between the
several files to make this infeasible?


It would be nice to get rid of all the code duplication in that file. 
That alone could reduce the size by quite a bit, and hopefully make it 
easier to read.


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



I'd suggest to do this shortly before GCC 6 is released, so that
backports from trunk to gcc-6-branch will be easy.  (I assume we don't
have to care for gcc-5-branch backports too much any longer.)


I'll declare myself agnostic as to whether such a change is appropriate 
for gcc-6 at this stage. I guess it kind of depends on the specifics.



Bernd