Re: [PATCH] omp-low.c split

[Martin Jambor <mjambor at suse dot cz>]

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 <mjambor@suse.cz> 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