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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
