[Openmp-commits] [PATCH] D59319: [OpenMP][Offloading][1/3] A generic and simple target region interface

Thu Mar 14 18:18:04 PDT 2019

jdoerfert added inline comments.

================
Comment at: openmp/libomptarget/deviceRTLs/common/target_region.h:100
+///
+EXTERN int8_t __kmpc_target_region_kernel_init(bool UseSPMDMode,
+                                               bool RequiresOMPRuntime,
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > ABataev wrote:
> > > > jdoerfert wrote:
> > > > > ABataev wrote:
> > > > > > jdoerfert wrote:
> > > > > > > ABataev wrote:
> > > > > > > > Better to use `ident_loc` for passing info about execution mode and full/lightweight runtime.
> > > > > > > Could you please explain why you think that? Adding indirection through a structure does not really seem beneficial to me.
> > > > > > Almost all function from libomp rely on `ident_loc`. The functions, which were added for NVPTX without this parameter had a lot of problems later and most of them were replaced with the functions with this parameter type. Plus, this parameter is used for OMPD/OMPT and it may be important for future OMPD/OMPT support.
> > > > > > Almost all function from libomp rely on ident_loc.
> > > > > 
> > > > > If you look at the implementation of this interface for NVPTX you will see that the called functions do not take `ident_loc` values. When you create the calls from the existing NVPTX code generation in clang, the current code **does not use** `ident_loc` for similar functions, see:
> > > > > `___kmpc_kernel_init(kmp_int32 thread_limit, int16_t RequiresOMPRuntime)`,
> > > > > `__kmpc_kernel_deinit(int16_t IsOMPRuntimeInitialized)`,
> > > > > `__kmpc_spmd_kernel_init(kmp_int32 thread_limit, int16_t RequiresOMPRuntime, int16_t RequiresDataSharing)`,
> > > > > `__kmpc_kernel_parallel(void **outlined_function, int16_t IsOMPRuntimeInitialized)`,
> > > > > ...
> > > > > 
> > > > > 
> > > > > 
> > > > > > Plus, this parameter is used for OMPD/OMPT and it may be important for future OMPD/OMPT support.
> > > > > 
> > > > > If we at some point need to make the options permanent in an `ident_loc` we can simply pass an `ident_loc` and require it to be initialized by the call. Cluttering the user code with stores and indirection is exactly what I do want to avoid.
> > > > 1. The new functions rely on `ident_loc`. We had to add those new functions because the old ones did not use it and it was bad design decision. Now we need to fix this. I suggest you do everything right from the very beginning rather than fixing this later by adding extra entry points to support OMPT/OMPD or something else, for example.
> > > > 2. No, you cannot simply change the interface of the library to keep the compatibility with the previous versions of the compiler/library. You will need to add the new entries.  
> > > Let's start this one again because I still haven't understood. Why do we need to populate the `ident_loc` again? What information has to be in there at which point? I want this to be clear because a lot of other "design decisions" of the existing code base are in my opinion not necessary and consequently missing here. That includes, for example, various global variables. If we have a description of the problem you try to solve with the `ident_loc` we might be able to find a way that cuts down on state.
> > > 
> > > 
> > > Regarding the "compatibility", this is not a stable interface people can rely on. Whatever is committed in this first patch __is not__ set in stone. Also, we can _always_ add a `__kmpc_init_ident_loc(....)` function after the fact.
> > Ident_loc holds the data about current source code location, execution mode and is full runtime required or not. Also, it is used in OMPT/OMPD support.
> > Regarding "compatibility" libraries must be most stable part of the compiler, because the user migbt need to link the old object file/library with the new one. Because of this the new versions of libraries must be compatible with old ones. And you need to maintain the deprecated parts to keep the compatibility with the previous versions. All these libs already have a lot of old code that because of the initial poor design and we need to maintain them. I would like to avoid this situation with this patch.
> > Ident_loc holds the data about current source code location, execution mode and is full runtime required or not. Also, it is used in OMPT/OMPD support.
> 
> We can store that information through a `__kmpc_init_ident_loc(....)` call once needed.
> 
> 
> > Regarding "compatibility" libraries must be most stable part of the compiler, because the user migbt need to link the old object file/library with the new one. Because of this the new versions of libraries must be compatible with old ones. And you need to maintain the deprecated parts to keep the compatibility with the previous versions. All these libs already have a lot of old code that because of the initial poor design and we need to maintain them. I would like to avoid this situation with this patch.
> 
> The way I understand you now is that you want a way to extend the interface in the future and adding a changeable `ident_loc` pointer is your proposed way. Do I understand your reaonsing for `ident_loc` here correctly or is it (this and) something else?
Addent ident_t everywhere.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59319/new/

https://reviews.llvm.org/D59319