Re: [PATCH] omp-low.c split

[Jakub Jelinek via Gcc-patches]

On Wed, Aug 04, 2021 at 02:40:27PM +0200, Thomas Schwinge wrote:
> Small fix-up for r243673 (Git commit 629b3d75c8c5a244d891a9c292bca6912d4b0dd9)
> "Split omp-low into multiple files".
> 
>   gcc/
>   * Makefile.in (GTFILES): Remove '$(srcdir)/omp-offload.c'.

Ok, thanks.
> ---
>  gcc/Makefile.in | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/gcc/Makefile.in b/gcc/Makefile.in
> index a9c9b506034..a3d9ee797df 100644
> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in
> @@ -2693,7 +2693,6 @@ GTFILES = $(CPPLIB_H) $(srcdir)/input.h 
> $(srcdir)/coretypes.h \
>$(srcdir)/tree-ssa-operands.h \
>$(srcdir)/tree-profile.c $(srcdir)/tree-nested.c \
>$(srcdir)/omp-offload.h \
> -  $(srcdir)/omp-offload.c \
>$(srcdir)/omp-general.c \
>$(srcdir)/omp-low.c \
>$(srcdir)/targhooks.c $(out_file) $(srcdir)/passes.c \
> -- 
> 2.30.2
> 


Jakub

Re: [PATCH] omp-low.c split

[Thomas Schwinge]

Hi!

On 2016-12-09T14:08:21+0100, Martin Jambor  wrote:
> this is the promised attempt at splitting omp-low.c [...]

> --- a/gcc/Makefile.in
> +++ b/gcc/Makefile.in

> @@ -2479,8 +2483,10 @@ GTFILES = $(CPP_ID_DATA_H) $(srcdir)/input.h 
> $(srcdir)/coretypes.h \
>$(srcdir)/tree-scalar-evolution.c \
>$(srcdir)/tree-ssa-operands.h \
>$(srcdir)/tree-profile.c $(srcdir)/tree-nested.c \
> +  $(srcdir)/omp-device.h \
> +  $(srcdir)/omp-device.c \
> +  $(srcdir)/omp-expand.c \
>$(srcdir)/omp-low.c \
> -  $(srcdir)/omp-low.h \
>$(srcdir)/targhooks.c $(out_file) $(srcdir)/passes.c 
> $(srcdir)/cgraphunit.c \
>$(srcdir)/cgraphclones.c \
>$(srcdir)/tree-phinodes.c \

'gcc/omp-device.*' eventually got renamed to 'gcc/omp-offload.*'.

OK to push the attached "Remove 'gcc/omp-offload.c' from 'GTFILES'"?

| Given that it doesn't contain any 'GTY' markers, no 'gcc/gt-omp-offload.h' 
file
| gets generated (and '#include'd anywhere).


Grüße
 Thomas


-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas 
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht 
München, HRB 106955
>From 1af32cf74a008a48328e82a6730b984f602b9979 Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Wed, 4 Aug 2021 13:41:22 +0200
Subject: [PATCH] Remove 'gcc/omp-offload.c' from 'GTFILES'

Given that it doesn't contain any 'GTY' markers, no 'gcc/gt-omp-offload.h' file
gets generated (and '#include'd anywhere).

Small fix-up for r243673 (Git commit 629b3d75c8c5a244d891a9c292bca6912d4b0dd9)
"Split omp-low into multiple files".

	gcc/
	* Makefile.in (GTFILES): Remove '$(srcdir)/omp-offload.c'.
---
 gcc/Makefile.in | 1 -
 1 file changed, 1 deletion(-)

diff --git a/gcc/Makefile.in b/gcc/Makefile.in
index a9c9b506034..a3d9ee797df 100644
--- a/gcc/Makefile.in
+++ b/gcc/Makefile.in
@@ -2693,7 +2693,6 @@ GTFILES = $(CPPLIB_H) $(srcdir)/input.h $(srcdir)/coretypes.h \
   $(srcdir)/tree-ssa-operands.h \
   $(srcdir)/tree-profile.c $(srcdir)/tree-nested.c \
   $(srcdir)/omp-offload.h \
-  $(srcdir)/omp-offload.c \
   $(srcdir)/omp-general.c \
   $(srcdir)/omp-low.c \
   $(srcdir)/targhooks.c $(out_file) $(srcdir)/passes.c \
-- 
2.30.2

Re: [PATCH] omp-low.c split

[Martin Jambor]

Hi,

On Wed, Dec 14, 2016 at 02:14:32PM +0100, Thomas Schwinge wrote:
> 
> I still couldn't allocate time to review the patch, but at least I now
> have tested it -- no regressions.

Great, thanks!

> As I suppose you want to commit this
> as sooner than later ;-) and you already have approval as I understand
> it, how about you do commit it, and I'll then later follow up, with any
> changes I'd additionally like to get done.
> 

I am about to re-base, re-test, add the few formatting fixes that
Jakub suggested to the second patch and commit.  Hopefully all of it
today.

Martin

Re: [PATCH] omp-low.c split

[Thomas Schwinge]

Hi!

On Tue, 13 Dec 2016 13:42:23 +0100, Martin Jambor  wrote:
> On Tue, Dec 13, 2016 at 12:43:16PM +0100, Jakub Jelinek wrote:
> > On Tue, Dec 13, 2016 at 12:39:01PM +0100, Thomas Schwinge wrote:
> > > On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor  wrote:
> > > > this is the promised attempt at splitting omp-low.c [...]

> > > >   - move all pre-lowering gridification stuff to a new file
> > > > omp-grid.c.  [...]
> > > 
> > > Is that code generic enough to not call this file "omp-hsa.c" or similar?
> > 
> 
> Not at the moment, but...
> 
> > And this as well.  But omp-grid.c is fine too.
> 
> ...I prefer omp-grid.c because I plan to use gridification also for
> GCN targets, though hopefully only as an optimization rather than a
> hard requirement ...and in fact I still think it is a good
> optimization of simple loops for execution on all CUDA-like
> environments with block/thread grids because it removes conditions
> which the run-time can handle better.

That certainly is reason enough to go with the generic name -- thanks for
the explanation!

> > > >   - I moved stuff that was used from all over the place to a new file
> > > > omp-general.c (unless it would mean exposing omp_region or
> > > > omp_context types).
> > > 
> > > I'd have called that simply "omp.c".
> > 
> > The problem with that is that the corresponding header can't be called
> > omp.h for obvious reasons, we already have one with very different meaning.
> 
> That is exactly the reason why I chose omp-general. 

OK, that wasn't obvious to me, but yeah, it's probably best to avoid the
name clash with libgomp's omp.h.


I still couldn't allocate time to review the patch, but at least I now
have tested it -- no regressions.  As I suppose you want to commit this
as sooner than later ;-) and you already have approval as I understand
it, how about you do commit it, and I'll then later follow up, with any
changes I'd additionally like to get done.


Grüße
 Thomas

Re: [PATCH] omp-low.c split

[Cesar Philippidis]

On 12/13/2016 04:42 AM, Martin Jambor wrote:

>> And this as well.  But omp-grid.c is fine too.
> 
> ...I prefer omp-grid.c because I plan to use gridification also for
> GCN targets, though hopefully only as an optimization rather than a
> hard requirement ...and in fact I still think it is a good
> optimization of simple loops for execution on all CUDA-like
> environments with block/thread grids because it removes conditions
> which the run-time can handle better.

Regarding gridification, is your cauldron talk from 2015 still current,
or have there been some significant changes?

When we first started with OpenACC we were using a lot of the existing
lower_omp_for* infrastructure handle ACC LOOPs. But there was a couple
of problems with that. First, the chunk partitioning caused a lot of
overhead, and second because of OpenACC execution model it made more
sense to write our own functions (lower_oacc_head_tail /
lower_oacc_reductions). In fact, during lowering gcc only marks where
the loops are. All of those markers get replaced and the loops get
optimized during the oaccdevlow pass which runs on the target compiler.

Right now one of the significant bottlenecks we're experiencing on nvptx
targets is with I/O. First, prior to launching a PTX kernel, libgomp
transfers each data mapping individually in a synchronous manner. I'm
debating whether it makes sense to pass in all of those data mappings to
the accelerator prior to the PTX kernel launch asynchronously, obviously
with an explicit synchronization barrier just prior to launching the kernel.

Another bottleneck involves firstprivate variables. Often, those
variables are 'scalars' and consequently, they shouldn't need explicit
data mappings. I noticed that Jakub introduced a special
GOMP_MAP_FIRSTPRIVATE_INT, which omits data mappings for integral types
with less than or equal precision to pointers. It would probably be
beneficial to expand this to reals.

The last observation is that OpenMP code in general passes a struct with
all of the data mappings to the various OMP regions/offloaded code.
That's fine, but for offloading targets, particularly nvptx, it would
probably be slightly more efficient if those OMP regions took actual
function arguments instead of a single struct. At least on nvptx
targets, in order to pass that struct to the accelerator, the runtime
must first allocate device memory for it, then copy all of the struct
contents to the device each time prior to launching a PTX kernel. A lot
of this could be bypassed because cuLaunchKernel accepts a variable
number of kernel arguments. Obviously, those arguments need to be
transferred to the accelerator one way or another, so I'm not sure yet
how beneficial this optimization would end up being.

To be clear, I'm not proposing any of these changes for gcc7. Any
changes to the above will go to gomp-4_0-branch first, then we'll port
them over to gcc8.

What type of performance problems are you experiencing with HSA?

Cesar

Re: [PATCH] omp-low.c split

[Alexander Monakov]

On Tue, 13 Dec 2016, Martin Jambor wrote:
> I have bootstrapped the two patches on aarch64-linux and bootstrapped
> and tested them on x86_64-linux.  What do you think?

Sorry for my 'false alarm' about cp/parser.c conflict in the previous mail -- I
thought I was applying your patch to trunk, but now I see my tree was outdated.

In the new patch there are 7 whitespace issues that git complains about:

zcat 0001-Split-omp-low-into-multiple-files.patch.gz |
  git apply --check --whitespace=error-all -

:2779: space before tab in indent.
# BLOCK 2 (PAR_ENTRY_BB)
:5391: space before tab in indent.
true, GSI_SAME_STMT);
:8174: space before tab in indent.
 after a stmt, not before.  */
:9105: space before tab in indent.
  GOMP_atomic_start ();
:9106: space before tab in indent.
  *addr = rhs;
:9107: space before tab in indent.
  GOMP_atomic_end ();
:10327: space before tab in indent.
 region.  */
error: 7 lines add whitespace errors.


A couple of typos in the Changelog:

>   (is_combined_parallel): kMoved to omp-expand.c.

s/k//

>   * config/nvptx/nvptx.c: Include omp-generic.c.

omp-general.h :)

Alexander

Re: [PATCH] omp-low.c split

[Martin Jambor]

Hi,

On Tue, Dec 13, 2016 at 12:43:16PM +0100, Jakub Jelinek wrote:
> On Tue, Dec 13, 2016 at 12:39:01PM +0100, Thomas Schwinge wrote:
> > On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor  wrote:
> > > this is the promised attempt at splitting omp-low.c [...]
> > 
> > Yay!  \o/
> > 
> > I have not yet had a chance to review/test this patch, but I plan to.
> > 
> > A few initial comments from the "bike shed departement"; I understand in
> > GCC sources it will not be easy to rename stuff (such as files) later, so
> > we should get the names agreed upon early:
> > 
> > Generally, I agree with your division of "omp-low.c" parts.
> > 
> > >   - move everything that is part of pass_oacc_device_lower,
> > > pass_omp_device_lower and pass_omp_target_link to a new file
> > > omp-device.h,
> > 
> > Should we call this file "omp-offload.c", as offloading is what this
> > deals with, is the term we agreed to generally use (as far as I can
> > tell)?
> 
> That would be fine with me too.

OK, will do.

> 
> > >   - move all pre-lowering gridification stuff to a new file
> > > omp-grid.c.  [...]
> > 
> > Is that code generic enough to not call this file "omp-hsa.c" or similar?
> 

Not at the moment, but...

> And this as well.  But omp-grid.c is fine too.

...I prefer omp-grid.c because I plan to use gridification also for
GCN targets, though hopefully only as an optimization rather than a
hard requirement ...and in fact I still think it is a good
optimization of simple loops for execution on all CUDA-like
environments with block/thread grids because it removes conditions
which the run-time can handle better.

> 
> > >   - I moved stuff that was used from all over the place to a new file
> > > omp-general.c (unless it would mean exposing omp_region or
> > > omp_context types).
> > 
> > I'd have called that simply "omp.c".
> 
> The problem with that is that the corresponding header can't be called
> omp.h for obvious reasons, we already have one with very different meaning.
> 

That is exactly the reason why I chose omp-general. 

Thanks,

Martin

Re: [PATCH] omp-low.c split

[Jakub Jelinek]

On Tue, Dec 13, 2016 at 12:39:01PM +0100, Thomas Schwinge wrote:
> On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor  wrote:
> > this is the promised attempt at splitting omp-low.c [...]
> 
> Yay!  \o/
> 
> I have not yet had a chance to review/test this patch, but I plan to.
> 
> A few initial comments from the "bike shed departement"; I understand in
> GCC sources it will not be easy to rename stuff (such as files) later, so
> we should get the names agreed upon early:
> 
> Generally, I agree with your division of "omp-low.c" parts.
> 
> >   - move everything that is part of pass_oacc_device_lower,
> > pass_omp_device_lower and pass_omp_target_link to a new file
> > omp-device.h,
> 
> Should we call this file "omp-offload.c", as offloading is what this
> deals with, is the term we agreed to generally use (as far as I can
> tell)?

That would be fine with me too.

> >   - move all pre-lowering gridification stuff to a new file
> > omp-grid.c.  [...]
> 
> Is that code generic enough to not call this file "omp-hsa.c" or similar?

And this as well.  But omp-grid.c is fine too.

> >   - I moved stuff that was used from all over the place to a new file
> > omp-general.c (unless it would mean exposing omp_region or
> > omp_context types).
> 
> I'd have called that simply "omp.c".

The problem with that is that the corresponding header can't be called
omp.h for obvious reasons, we already have one with very different meaning.

Jakub

Re: [PATCH] omp-low.c split

[Thomas Schwinge]

Hi!

On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor  wrote:
> this is the promised attempt at splitting omp-low.c [...]

Yay!  \o/

I have not yet had a chance to review/test this patch, but I plan to.

A few initial comments from the "bike shed departement"; I understand in
GCC sources it will not be easy to rename stuff (such as files) later, so
we should get the names agreed upon early:

Generally, I agree with your division of "omp-low.c" parts.

>   - move everything that is part of pass_oacc_device_lower,
> pass_omp_device_lower and pass_omp_target_link to a new file
> omp-device.h,

Should we call this file "omp-offload.c", as offloading is what this
deals with, is the term we agreed to generally use (as far as I can
tell)?

>   - move all pre-lowering gridification stuff to a new file
> omp-grid.c.  [...]

Is that code generic enough to not call this file "omp-hsa.c" or similar?

>   - I moved stuff that was used from all over the place to a new file
> omp-general.c (unless it would mean exposing omp_region or
> omp_context types).

I'd have called that simply "omp.c".

> I am opened to suggestions what to do differently, names of the file
> are for example of course subject to discussion, and I absolutely
> welcome any review and checking, for one I am not going to pretend I
> understand the stuff I put into omp-device.c.  If however there is
> consensus that we should do something like this, I would like to ask
> the community to freeze omp-low.c file until this gets committed, I
> hope you understand that I am afraid of any conflicts.

I very much understand...  :-| When I had worked on the very same thing
months ago, and my changes went without review/approval for a long time,
I had spent numerous hours on keeping my patch up to date.  So, I'm happy
to see that this is now near approval!  (Even though I don't understand
what's different now from when I worked on the same thing back then...)

I hope to have time later today to review/test your actual patch.


Grüße
 Thomas

Re: [PATCH] omp-low.c split

[Jakub Jelinek]

On Tue, Dec 13, 2016 at 11:15:43AM +0100, Martin Jambor wrote:
> I have bootstrapped the two patches on aarch64-linux and bootstrapped
> and tested them on x86_64-linux.  What do you think?

Thanks a lot for the work.  If you wouldn't mind doing a couple of further
changes (see below), I'd appreciate it, but if you want to commit it right
away, I'm ok with that too.

@@ -4321,7 +4322,7 @@ expand_cilk_for (struct omp_region *region, struct 
omp_for_data *fd)
 
   tree child_fndecl
 = gimple_omp_parallel_child_fn (
-as_a  (last_stmt (region->outer->entry)));
+   as_a  (last_stmt (region->outer->entry)));
   tree t, low_val = NULL_TREE, high_val = NULL_TREE;
   for (t = DECL_ARGUMENTS (child_fndecl); t; t = TREE_CHAIN (t))
 {

My preference for the above would be

  gomp_parallel *par_stmt
= as_a  (last_stmt (region->outer->entry));
  tree child_fndecl = gimple_omp_parallel_child_fn (par_stmt);

@@ -6428,7 +6429,7 @@ expand_omp_atomic_pipeline (basic_block load_bb, 
basic_block store_bb,
  floating point.  This allows the atomic operation to properly
  succeed even with NaNs and -0.0.  */
   stmt = gimple_build_cond_empty
-   (build2 (NE_EXPR, boolean_type_node,
+  (build2 (NE_EXPR, boolean_type_node,
new_storedi, old_vali));
   gsi_insert_before (, stmt, GSI_SAME_STMT);
 
And here

  tree ne = build2 (NE_EXPR, boolean_type_node, new_storedi, old_vali);
  stmt = gimple_build_cond_empty (ne);

@@ -442,7 +442,7 @@ omp_max_simt_vf (void)
   if (!optimize)
 return 0;
   if (ENABLE_OFFLOADING)
-for (const char *c = getenv ("OFFLOAD_TARGET_NAMES"); c; )
+for (const char *c = getenv ("OFFLOAD_TARGET_NAMES"); c;)

I admit I don't know what the coding style we have in this case.  Ok either way.

@@ -5860,7 +5860,7 @@ lower_omp_sections (gimple_stmt_iterator *gsi_p, 
omp_context *ctx)
   new_body = maybe_catch_exception (new_body);
 
   t = gimple_build_omp_return
-(!!omp_find_clause (gimple_omp_sections_clauses (stmt),
+   (!!omp_find_clause (gimple_omp_sections_clauses (stmt),
OMP_CLAUSE_NOWAIT));
   gimple_seq_add_stmt (_body, t);
   maybe_add_implicit_barrier_cancel (ctx, _body);
@@ -6023,7 +6023,7 @@ lower_omp_single (gimple_stmt_iterator *gsi_p, 
omp_context *ctx)
   bind_body = maybe_catch_exception (bind_body);
 
   t = gimple_build_omp_return
-(!!omp_find_clause (gimple_omp_single_clauses (single_stmt),
+   (!!omp_find_clause (gimple_omp_single_clauses (single_stmt),
OMP_CLAUSE_NOWAIT));
   gimple_seq_add_stmt (_body_tail, t);
   maybe_add_implicit_barrier_cancel (ctx, _body_tail);

And in the above 2 spots something like:
  bool nowait = omp_find_clause (gimple_omp_single_clauses (single_stmt),
 OMP_CLAUSE_NOWAIT) != NULL_TREE;
  t = gimple_build_omp_return (nowait);

(wonder why we use t for gimple, renaming t var to g would be also nice).  But
ok either way.

@@ -8668,7 +8669,7 @@ lower_omp_1 (gimple_stmt_iterator *gsi_p, omp_context 
*ctx)
   break;
 case GIMPLE_TRANSACTION:
   lower_omp (gimple_transaction_body_ptr (
-   as_a  (stmt)),
+  as_a  (stmt)),
 ctx);

Here it fits, so no need to wrap:
  lower_omp (gimple_transaction_body_ptr (as_a  (stmt)),
 ctx);


Jakub

Re: [PATCH] omp-low.c split

[Martin Jambor]

Hi,

On Fri, Dec 09, 2016 at 07:18:54PM +0300, Alexander Monakov wrote:
> On Fri, 9 Dec 2016, Jakub Jelinek wrote:
> > Can you post an incremental patch fixing those issues?
> 
> A few small nits I found while reading the patch.
> 
> First of all, please use 'git diff --patience' (or --histogram) when
> generating such patches, without it the changes in omp-low.c look uglier than
> necessary.

OK, the new patches use --patience, the result is actually quite a bit
smaller.  Thanks for the hint.

> 
> The comment for 'struct oacc_loop' in new file omp-device.c is misplaced: for
> some reason it's above the #include block.

Oh, thanks for noticing, fixed.

> 
> This patch doesn't seem to apply to current trunk due to a conflict in
> cp/parser.c.

Hmm, I don't think so?

  $ svn st
  $ svn up
  Updating '.':
  At revision 243600.
  $ patch --dry-run -p1 < 
/tmp/misc-next/pat/0001-Split-omp-low-into-multiple-files.patch
  checking file gcc/Makefile.in
  checking file gcc/c-family/c-omp.c
  checking file gcc/c/c-parser.c
  checking file gcc/c/c-typeck.c
  checking file gcc/c/gimple-parser.c
  checking file gcc/config/nvptx/nvptx.c
  checking file gcc/cp/parser.c
  checking file gcc/cp/semantics.c
  checking file gcc/fortran/trans-openmp.c
  checking file gcc/gengtype.c
  checking file gcc/gimple-fold.c
  checking file gcc/gimplify.c
  checking file gcc/lto-cgraph.c
  checking file gcc/omp-device.c
  checking file gcc/omp-device.h
  checking file gcc/omp-expand.c
  checking file gcc/omp-expand.h
  checking file gcc/omp-general.c
  checking file gcc/omp-general.h
  checking file gcc/omp-grid.c
  checking file gcc/omp-grid.h
  checking file gcc/omp-low.c
  checking file gcc/omp-low.h
  checking file gcc/toplev.c
  checking file gcc/tree-cfg.c
  checking file gcc/tree-parloops.c
  checking file gcc/tree-ssa-loop.c
  checking file gcc/tree-vrp.c
  checking file gcc/varpool.c
  $ echo $?
  0

This was using the exact same file I have compressed and sent a while
ago to the mailing list.

> If you could create a git branch (perhaps in your personal
> namespace so you can freely rebase it) with this patchset, I'd appreciate it.
> 

Well... I do not see this as a long-term project so I do not really
see much value in this.  I have used git just because that is how I
conveniently send patches to machines where I bootstrap them.  I hope
this will get committed very early so that we avoid any big conflicts
in omp-low.c.  But if this drags on for a while and if our git mirror
recovers meanwhile, I can.


> When trying to apply the patch, git notes a few remaining whitespace issues:
> 
> $ zcat 0001-Split-omp-low-into-multiple-files.patch.gz | git apply --reject -
> 
> :2734: space before tab in indent.
> # BLOCK 2 (PAR_ENTRY_BB)
> :5346: space before tab in indent.
> true, GSI_SAME_STMT);
> :8129: space before tab in indent.
>  after a stmt, not before.  */
> :9060: space before tab in indent.
>   GOMP_atomic_start ();
> :9061: space before tab in indent.
>   *addr = rhs;
> 

I hope I have addressed this when doing what Jakub suggested, please
let me know if not.

Thanks for looking at the big patch!

Martin

Re: [PATCH] omp-low.c split

[Alexander Monakov]

On Fri, 9 Dec 2016, Jakub Jelinek wrote:
> Can you post an incremental patch fixing those issues?

A few small nits I found while reading the patch.

First of all, please use 'git diff --patience' (or --histogram) when
generating such patches, without it the changes in omp-low.c look uglier than
necessary.

The comment for 'struct oacc_loop' in new file omp-device.c is misplaced: for
some reason it's above the #include block.

This patch doesn't seem to apply to current trunk due to a conflict in
cp/parser.c.  If you could create a git branch (perhaps in your personal
namespace so you can freely rebase it) with this patchset, I'd appreciate it.

When trying to apply the patch, git notes a few remaining whitespace issues:

$ zcat 0001-Split-omp-low-into-multiple-files.patch.gz | git apply --reject -

:2734: space before tab in indent.
# BLOCK 2 (PAR_ENTRY_BB)
:5346: space before tab in indent.
true, GSI_SAME_STMT);
:8129: space before tab in indent.
 after a stmt, not before.  */
:9060: space before tab in indent.
  GOMP_atomic_start ();
:9061: space before tab in indent.
  *addr = rhs;


Hope this helps.
Alexander

Re: [PATCH] omp-low.c split

[Jakub Jelinek]

On Fri, Dec 09, 2016 at 02:53:41PM +0100, Martin Jambor wrote:
> Unfortunately no, that file also needs to be changed, even if very
> slightly.  Specifically, omp-general.h also needs to be included and
> calls to get_oacc_fn_attrib need to be changed to call
> oacc_get_fn_attrib.  omp-low.h has to stay included for
> omp_reduction_init_op and omp_reduction_init which did not change.
> 
> Sorry about that, it was the only file in the back-ends and I forgot
> about it.  I have added the following to my patch but it would be
> great if you verified it still compiles and works as expected for you.

The patch including this looks mostly good to me, but can we take it
also as an opportunity to clean up formatting where it went wrong over the
years?
contrib/check_GNU_style.sh 0001-Split-omp-low-into-multiple-files.patch
reports lots of issues, as always, the script is not perfect and one needs
to use reasonable judgement.
There are e.g. 9 lines with over 80 chars, 19 lines with blocks of 8 spaces
instead of tabs, some trailing whitespaces, "Dot, space, space, new
sentence", "Dot, space, space, end of comment", some of the
"There should be exactly one space between function name and parenthesis"
- for omp directives in comments it should stay as is, but
oacc_loop_sibling_nreverse  (loop->child);
if (__builtin_expect(zero, false)) goto zero_iter_bb;
tgt = _CLAUSE_CHAIN(c);
should be fixed.
Space before dot are all false positives, etc.
Trailing operator seems to also have 2 real bugs.

Can you post an incremental patch fixing those issues?

Thanks.

Jakub

Re: [PATCH] omp-low.c split

[Martin Jambor]

Hi,

On Fri, Dec 09, 2016 at 04:25:10PM +0300, Alexander Monakov wrote:
> Hi Martin,
> 
> Just one quick question -- do you know if config/nvptx/nvptx.c needs changes
> with this patch?  I see it has an '#include "omp-low.h"', and it seems your
> patch is renaming some functions -- is the intention that no interfaces used 
> in
> target-specific files are changed during the split?
> 

Unfortunately no, that file also needs to be changed, even if very
slightly.  Specifically, omp-general.h also needs to be included and
calls to get_oacc_fn_attrib need to be changed to call
oacc_get_fn_attrib.  omp-low.h has to stay included for
omp_reduction_init_op and omp_reduction_init which did not change.

Sorry about that, it was the only file in the back-ends and I forgot
about it.  I have added the following to my patch but it would be
great if you verified it still compiles and works as expected for you.

Thanks,

Martin


* config/nvptx/nvptx.c: Include omp-generic.c.
(nvptx_expand_call): Adjusted the call to get_oacc_fn_attrib to use
its new name.
(nvptx_reorg): Likewise.
(nvptx_record_offload_symbol): Likewise.


diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c
index 405a91b..17fe551 100644
--- a/gcc/config/nvptx/nvptx.c
+++ b/gcc/config/nvptx/nvptx.c
@@ -55,6 +55,7 @@
 #include "gimple.h"
 #include "stor-layout.h"
 #include "builtins.h"
+#include "omp-general.h"
 #include "omp-low.h"
 #include "gomp-constants.h"
 #include "dumpfile.h"
@@ -1389,7 +1390,7 @@ nvptx_expand_call (rtx retval, rtx address)
  if (DECL_STATIC_CHAIN (decl))
cfun->machine->has_chain = true;
 
- tree attr = get_oacc_fn_attrib (decl);
+ tree attr = oacc_get_fn_attrib (decl);
  if (attr)
{
  tree dims = TREE_VALUE (attr);
@@ -4090,7 +4091,7 @@ nvptx_reorg (void)
   /* Determine launch dimensions of the function.  If it is not an
  offloaded function  (i.e. this is a regular compiler), the
  function has no neutering.  */
-  tree attr = get_oacc_fn_attrib (current_function_decl);
+  tree attr = oacc_get_fn_attrib (current_function_decl);
   if (attr)
 {
   /* If we determined this mask before RTL expansion, we could
@@ -4243,7 +4244,7 @@ nvptx_record_offload_symbol (tree decl)
 
 case FUNCTION_DECL:
   {
-   tree attr = get_oacc_fn_attrib (decl);
+   tree attr = oacc_get_fn_attrib (decl);
/* OpenMP offloading does not set this attribute.  */
tree dims = attr ? TREE_VALUE (attr) : NULL_TREE;
 

Re: [PATCH] omp-low.c split

[Alexander Monakov]

Hi Martin,

Just one quick question -- do you know if config/nvptx/nvptx.c needs changes
with this patch?  I see it has an '#include "omp-low.h"', and it seems your
patch is renaming some functions -- is the intention that no interfaces used in
target-specific files are changed during the split?

Thanks.
Alexander